rnk added subscribers: aganea, mikerice.
rnk added a comment.
Honestly, MSVC's behavior makes more sense to me. Usually warnings tell the
user they are doing something silly, and then let them do it anyway. Without
this change, there is no way to add extra macros to some compilations, even if
they won't affect the already-parsed PCH blob.
+@aganea, since I think he has used /Yc /Yu, and @mikerice, the last person to
touch it.
BTW, this raises the issue of how PCH will work with dllexport, which typically
works by defining some kind of "FOO_DLL" macro to indicate that some
annotations should expand to `__declspec(dllexport)`. I suppose this change
won't solve that problem, but it's still interesting.
================
Comment at: clang/lib/Lex/PPDirectives.cpp:2728
/*Syntactic=*/LangOpts.MicrosoftExt))
Diag(MI->getDefinitionLoc(), diag::warn_pp_macro_def_mismatch_with_pch)
<< MacroNameTok.getIdentifierInfo();
----------------
I think the case of passing -D to compilation but not PCH is likely to be the
overwhelming majority of cases that users run into, and MSVC has a nice,
specific diagnostic for that case (probably "!OtherMI"). We should do that too,
but do not feel obligated to do that in this change.
================
Comment at: clang/lib/Lex/PPDirectives.cpp:2731
+ // Issue the diagnostic but allow the change under msvc compatability mode
+ if (!getLangOpts().MSVCCompat)
+ return;
----------------
PCH is in the space of implementation-defined behaviors, not standards-mandated
behaviors. I think MicrosoftExt (-fms-extensions) would be a better condition
here. Besides, it's consistent with the use just above.
================
Comment at: clang/test/PCH/fuzzy-pch-msvc.c:30-31
+
+// CHECK-FOO: definition of macro 'FOO' differs between the precompiled header
('1') and the command line ('blah')
+// CHECK-NOFOO: macro 'FOO' was defined in the precompiled header but undef'd
on the command line
+// CHECK-BAR: error: definition of macro 'BAR' does not match definition in
precompiled header [-Werror,-Wclang-cl-pch]
----------------
For this stuff, it would be nicer to use `%clang_cc1 -verify` instead of
FileCheck. You can do things like:
```
// expected-warning@2 {{'FOO' macro redefined}}
// expected-note@1 {{previous definition is here}}
int main() {}
```
I just checked, and that works.
I see that the fuzzy-pch.c test that you based this on probably predates clang
-cc1 -verify.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72405/new/
https://reviews.llvm.org/D72405
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
