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?
On Tue, Sep 20, 2016 at 12:30 PM, Nico Weber <tha...@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 <agutow...@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>}}
>>
>>
>>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
