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

[Albert Gutowski via cfe-commits]

Thanks for clarification!
You are right that it doesn't really fit in -Wignored-pragma, but I
also don't like the idea of getting warning about ignored "#pragma
intrinsic" after setting -Wno-ignored-pragma, so I think I'll leave it
as it is.

On Wed, Sep 21, 2016 at 1:35 PM, Nico Weber <thakis at chromium.org> wrote:
> 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>}}
>>>>
>>>>
>>>
>>
>