[PATCH] D51391: [clang-cl,PCH] Add support for #pragma hdrstop

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Very cool! I only have some minor comments.

Oh, and when it lands, you should probably add an update to docs/ReleaseNotes.rst about it too.



================
Comment at: include/clang/Driver/CC1Options.td:613
+  HelpText<"Stop PCH generation after #pragma hdrstop.  When using a PCH, "
+           "skip tokens until after a #pragma hdrstop">;
 def fno_pch_timestamp : Flag<["-"], "fno-pch-timestamp">,
----------------
should this mention the argument is supposed to be "create" or "use"?


================
Comment at: lib/Driver/Driver.cpp:2987
   // Diagnose unsupported forms of /Yc /Yu. Ignore /Yc/Yu for now if:
   // * no filename after it
   // * both /Yc and /Yu passed but with different filenames
----------------
I think this part of the comment no longer applies.


================
Comment at: lib/Driver/Driver.cpp:4274
   } else {
-    Output = BaseName;
+    Arg *YcArg = C.getArgs().getLastArg(options::OPT__SLASH_Yc);
+    if (YcArg)
----------------
The declaration of YcArg could still be part of the if-statement below, I think. It doesn't look like you're using it outside the if.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1112
+            Twine("-pch-through-hdrstop=") + (YcArg ? "create" : "use")));
+      } else
+        CmdArgs.push_back(
----------------
nit: i'd probably use a curly brace here since the statement below doesn't fit on one line. maybe that's just me though.


================
Comment at: lib/Frontend/CompilerInvocation.cpp:2862
+  Opts.PCHWithHdrStopCreate =
+      Args.getLastArgValue(OPT_pch_through_hdrstop_EQ) == "create";
   Opts.PCHThroughHeader = Args.getLastArgValue(OPT_pch_through_header_EQ);
----------------
hmm, what if the value is not "create", but also not "use" but something else? maybe that should be diagnosed, or maybe the flag should be split into two?


================
Comment at: lib/Lex/Pragma.cpp:904
+    CurLexer->FormTokenWithChars(Result, CurLexer->BufferEnd, tok::eof);
+    CurLexer->cutOffLexing();
+  }
----------------
Nice!


================
Comment at: test/Driver/cl-pch.cpp:280
+// CHECK-YC-NOARG: cl-pch.cpp
+// 2. Use .pch file: Includes ycnoarg.pch
+// CHECK-YC-NOARG: cc1
----------------
i suppose if there's a "2." comment, might as well have a comment for step 1 too.  same for "/Yc with no argument and no /FP" below


https://reviews.llvm.org/D51391