[PATCH] D72405: Allow /D flags absent during PCH creation under msvc-compat

[Reid Kleckner via Phabricator via cfe-commits]

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.

+ at 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 at 2 {{'FOO' macro redefined}}
// expected-note at 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