[PATCH] D46652: [clang-cl, PCH] Implement support for MS-style PCH through headers

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

This looks reasonable to me as far as I can tell. Thanks!

I think the expert on the driver side for this stuff is Nico, so hopefully he can take a look as well.



================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:412
+  "%select{create|use}1 precompiled header">, DefaultFatal;
+def err_pp_macro_def_mismatch_with_pch : Warning<
+  "definition of macro %0 does not match definition in precompiled header">,
----------------
Should probably just have a warn_ prefix.


================
Comment at: include/clang/Driver/CC1Options.td:604
+  HelpText<"When creating a pch stop at this file.  When using a pch start "
+           "after this file.">;
 def fno_pch_timestamp : Flag<["-"], "fno-pch-timestamp">,
----------------
The "through header" terminology was new to me, and I didn't see it when browsing the MSDN articles about precompiled headers. The HelpText here probably isn't the right place, but it would be good if the term could be documented somewhere to make it clearer exactly what the behaviour is. It's not really obvious to me what "stop at this file" and "start after this file"  means. I can guess, but it would be nice if it were more explicit :-)


================
Comment at: include/clang/Lex/Preprocessor.h:2050
+    PCHThroughHeaderFileID = FID;
+  }
+
----------------
I would probably define this out-of-line like the other methods you added, but no big deal.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1091
+          Args.MakeArgString(Twine("-pch-through-header=") + ThroughHeader));
     }
   }
----------------
In r335466, I added the -building-pch-with-obj flag. This is probably the right place to pass it when there's a /Yc flag.


================
Comment at: lib/Lex/PPDirectives.cpp:1887
+      SkippingUntilPCHThroughHeader = false;
+    return;
+  }
----------------
Returning here seemed surprising to me. Isn't just setting the flag and then carrying on as usual what we want?


================
Comment at: lib/Lex/PPLexerChange.cpp:344
   const bool LeavingSubmodule = CurLexer && CurLexerSubmodule;
+  bool LeavingPCHThroughHeader = false;
   if ((LeavingSubmodule || IncludeMacroStack.empty()) &&
----------------
Maybe move this down to where it's first used?


================
Comment at: lib/Lex/Preprocessor.cpp:583
+
+/// Return true if the FileEntry is the PCH through header.
+bool Preprocessor::isPCHThroughHeader(const FileEntry *FE) {
----------------
I know some functions already do this, but I don't think repeating the comment from the .h file here is necessary, it's just another thing to keep in sync. Same for the other small methods below.


================
Comment at: lib/Lex/Preprocessor.cpp:607
+void Preprocessor::SkipTokensUntilPCHThroughHeader()
+{
+  bool ReachedMainFileEOF = false;
----------------
nit: curly on the previous line


================
Comment at: test/PCH/pch-through2.cpp:3
+// RUN: %clang_cc1 -I %S -emit-pch \
+// RUN:   -pch-through-header=Inputs/pch-through2.h -o %t.s2t2 %s
+
----------------
What's s2t2 for? From what I see, there's only two pch files used in this test, so I would have gone with %t.1 and %t.2


https://reviews.llvm.org/D46652