This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fix message text for `-Wpointer-sign` to account for plain char
ClosedPublic

Authored by hubert.reinterpretcast on Jan 3 2021, 1:37 PM.

Details

Summary

The -Wpointer-sign warning text is inappropriate for describing the incompatible pointer conversion between plain char and explicitly signed/unsigned char (whichever plain char has the same range as) and vice versa.

Specifically, in part, it reads "converts between pointers to integer types with different sign". This patch changes that portion to read instead as "converts between pointers to integer types where one is of the unique plain char type and the other is not" when one of the types is plain char.

C17 subclause 6.5.16.1 indicates that the conversions resulting in -Wpointer-sign warnings in assignment-like contexts are constraint violations. This means that strict conformance requires a diagnostic for the case where the message text is wrong before this patch. The lack of an even more specialized warning group is consistent with GCC.

Diff Detail

Event Timeline

hubert.reinterpretcast requested review of this revision.Jan 3 2021, 1:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2021, 1:37 PM
aaron.ballman added inline comments.Jan 8 2021, 8:51 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7817–7819

I fee like that differ by signed/unsigned/plain variation is a bit hard for users to understand and maybe we want to spell it out a bit more explicitly. I took a stab at a more wordy diagnostic that I think is easier to understand, but if you have other ideas, I'm not tied to my wording. WDYT?

(Same suggestion applies below -- though we may want to switch to ext_typecheck_convert_incompatible_pointer_sign.Text below rather than spelling all this out manually.)

clang/include/clang/Basic/DiagnosticSemaKinds.td
7817–7819

I think the "other excludes sign information" wording makes this sound like a portability diagnostic. I was going for wording that retains the same "seriousness" for the plain char versus signed/unsigned char case as for the signed char versus unsigned char case.

Would "where one is of the unique plain char type and the other is not" work?

aaron.ballman added inline comments.Jan 8 2021, 11:38 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7817–7819

Would "where one is of the unique plain char type and the other is not" work?

Yeah, I think that's great wording, thank you!

  • Address review: Mention plain char only when it appears
hubert.reinterpretcast edited the summary of this revision. (Show Details)Jan 9 2021, 8:56 PM
hubert.reinterpretcast marked 2 inline comments as done.
aaron.ballman accepted this revision.Jan 11 2021, 6:12 AM

LGTM aside from some very minor nits.

clang/include/clang/Basic/DiagnosticSemaKinds.td
7818
clang/lib/Sema/SemaExpr.cpp
15967

We don't typically use top-level const on value types for local variables.

This revision is now accepted and ready to land.Jan 11 2021, 6:12 AM
hubert.reinterpretcast marked 2 inline comments as done.Jan 11 2021, 4:04 PM