[PATCH] D24775: Add -Wignored-pragma-intrinsic flag

[Nico Weber via cfe-commits]

On Tue, Sep 20, 2016 at 4:21 PM, Albert Gutowski <agutowski at google.com>
wrote:

> OK, thanks for the note about referring to Chromium, I fixed that.
> As to -Wunknown-pragma, I feel that it would be inconsistent with other
> pragmas unless I moved whole pragma to lexer instead of parser - I've just
> discovered that I can do that, how should I decide if it's supposed to be
> here or there?
>

Your commit reminds me that I forgot to reply to this thread. For the
lexer/parser split, I think about it like this: If clang doesn't know about
a certain pragma at all, it can't do anything with it, so all it does is
emit 'unknown pragma', skip it, and move on. If it does know the pragma, it
can parse it -- but then it might have to ignore it for some reason, which
is why the parser prints that diagnostic.

So you're right that this doesn't fit in well in -Wunknown-pragma.

But -Wignored-pragma doesn't quite fit either imho, since we're not
ignoring the pragma, we're looking at its context.

Applying the same idea for pragmas to `#pragma intrinsic`, it feels to me
that we don't know some intrinsics at all, so they're more unknown than
ignored (as in -Wunkown-pragma-intrinsic, but in the sense of as "unknown
(pragma intrinsic)" not "(unknown pragma) intrinsic" -- but since this
shouldn't be in -Wunknown-pragma as you correctly say, that's not a good
name for this flag.)

So I think -Wmicrosoft-pragma-intrinsic might be a better fit. (But as I
said, it's bikeshedding, and up to you.)


>
> On Tue, Sep 20, 2016 at 12:30 PM, Nico Weber <thakis at chromium.org> wrote:
>
>> Thanks! Some bikesheddy comments, ignore as you see fit:
>>
>> - I think it's good to keep Chromium's bug tracker out of LLVM as far as
>> possible (most LLVM devs don't need to care about Chromium, and since
>> Chromium's bug tracker refers to LLVM's bug tracker frequently since
>> chromium depends on llvm, adding links the other way round adds a
>> dependency cycle of sorts), so maybe describe the motivation in your own
>> words instead of linking ("People might want to receive warnings about
>> pragmas but not about intrinsics we don't yet implement")
>>
>> - Should this be part of -Wunknown-pragma instead of -Wignored-pragma?
>> (Or maybe even in -Wmicrosoft somewhere?) That seems to fit a tiny bit
>> better imho (but as I said, feel free to disagree and ignore)
>>
>> On Tue, Sep 20, 2016 at 2:38 PM, Albert Gutowski <agutowski at google.com>
>> wrote:
>>
>>> agutowski created this revision.
>>> agutowski added reviewers: thakis, hans.
>>> agutowski added a subscriber: cfe-commits.
>>>
>>> https://bugs.chromium.org/p/chromium/issues/detail?id=644841#c9
>>>
>>> https://reviews.llvm.org/D24775
>>>
>>> Files:
>>>   include/clang/Basic/DiagnosticGroups.td
>>>   include/clang/Basic/DiagnosticParseKinds.td
>>>   test/Preprocessor/pragma_microsoft.c
>>>
>>> Index: include/clang/Basic/DiagnosticParseKinds.td
>>> ===================================================================
>>> --- include/clang/Basic/DiagnosticParseKinds.td
>>> +++ include/clang/Basic/DiagnosticParseKinds.td
>>> @@ -914,7 +914,7 @@
>>>  // - #pragma intrinsic
>>>  def warn_pragma_intrinsic_builtin : Warning<
>>>    "%0 is not a recognized builtin%select{|; consider including
>>> <intrin.h> to access non-builtin intrinsics}1">,
>>> -  InGroup<IgnoredPragmas>;
>>> +  InGroup<IgnoredPragmaIntrinsic>;
>>>  // - #pragma unused
>>>  def warn_pragma_unused_expected_var : Warning<
>>>    "expected '#pragma unused' argument to be a variable name">,
>>> Index: include/clang/Basic/DiagnosticGroups.td
>>> ===================================================================
>>> --- include/clang/Basic/DiagnosticGroups.td
>>> +++ include/clang/Basic/DiagnosticGroups.td
>>> @@ -439,8 +439,9 @@
>>>  def UninitializedStaticSelfInit : DiagGroup<"static-self-init">;
>>>  def Uninitialized  : DiagGroup<"uninitialized", [UninitializedSometimes,
>>>
>>> UninitializedStaticSelfInit]>;
>>> +def IgnoredPragmaIntrinsic : DiagGroup<"ignored-pragma-intrinsic">;
>>>  def UnknownPragmas : DiagGroup<"unknown-pragmas">;
>>> -def IgnoredPragmas : DiagGroup<"ignored-pragmas">;
>>> +def IgnoredPragmas : DiagGroup<"ignored-pragmas",
>>> [IgnoredPragmaIntrinsic]>;
>>>  def Pragmas : DiagGroup<"pragmas", [UnknownPragmas, IgnoredPragmas]>;
>>>  def UnknownWarningOption : DiagGroup<"unknown-warning-option">;
>>>  def NSobjectAttribute : DiagGroup<"NSObject-attribute">;
>>> Index: test/Preprocessor/pragma_microsoft.c
>>> ===================================================================
>>> --- test/Preprocessor/pragma_microsoft.c
>>> +++ test/Preprocessor/pragma_microsoft.c
>>> @@ -178,3 +178,15 @@
>>>  #pragma intrinsic(memset) // no-warning
>>>  #undef __INTRIN_H
>>>  #pragma intrinsic(asdf) // expected-warning {{'asdf' is not a
>>> recognized builtin; consider including <intrin.h>}}
>>> +
>>> +#pragma clang diagnostic push
>>> +#pragma clang diagnostic ignored "-Wignored-pragma-intrinsic"
>>> +#pragma intrinsic(asdf) // no-warning
>>> +#pragma clang diagnostic pop
>>> +#pragma intrinsic(asdf) // expected-warning {{'asdf' is not a
>>> recognized builtin; consider including <intrin.h>}}
>>> +
>>> +#pragma clang diagnostic push
>>> +#pragma clang diagnostic ignored "-Wignored-pragmas"
>>> +#pragma intrinsic(asdf) // no-warning
>>> +#pragma clang diagnostic pop
>>> +#pragma intrinsic(asdf) // expected-warning {{'asdf' is not a
>>> recognized builtin; consider including <intrin.h>}}
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160921/15f035dd/attachment-0001.html>