[PATCH] D17695: clang-cl: Implement initial limited support for precompiled headers.

[Hans Wennborg via cfe-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

Very nice!

I only have some style nits, really. Look forward to using this :-)


================
Comment at: include/clang/Basic/DiagnosticDriverKinds.td:112
@@ +111,3 @@
+def warn_drv_yc_multiple_inputs_clang_cl : Warning<
+  "support for '/Yc' with more than one input source file not implemented yet, ignoring flag">,
+  InGroup<ClangClPch>;
----------------
nit: I think it's more common in Clang's warnings to say "ignored" than "ignoring", and "flag ignored" saves one character :-)

Also I would have gone for a semicolon instead of comma, but this is not important.

Maybe "input" is redundant in "more than one input source file"?

</wordsmithing>

================
Comment at: include/clang/Driver/CLCompatOptions.td:258
@@ +257,3 @@
+def _SLASH_Yu : CLJoined<"Yu">;
+def _SLASH_Fp : CLJoined<"Fp">;
+
----------------
Can you add HelpText for these? I've tried to be good about documenting supported cl-mode flags.

================
Comment at: lib/Driver/Driver.cpp:1448
@@ +1447,3 @@
+  Arg *YuArg = Args.getLastArg(options::OPT__SLASH_Yu);
+  if (YcArg && YcArg->getValue()[0] == '\0') {
+    Diag(clang::diag::warn_drv_ycyu_no_arg_clang_cl) << YcArg->getSpelling();
----------------
A bit surprised that getValue() doesn't return a StringRef. But given that it doesn't, I suppose this is the nicest way to write it.

================
Comment at: lib/Driver/Driver.cpp:1466
@@ +1465,3 @@
+    StringRef Val = YcArg ? YcArg->getValue() : YuArg->getValue();
+    bool Found = false;
+    for (const Arg *Inc : Args.filtered(options::OPT_include)) {
----------------
FoundMatchingInclude?

================
Comment at: lib/Driver/Driver.cpp:2178
@@ -2085,3 +2177,3 @@
       NamedOutput = C.getArgs().MakeArgString(Output.c_str());
     } else
       NamedOutput = getDefaultImageName();
----------------
maybe add curlies around this else statement to make it a little easier to parse the else if that comes after?

================
Comment at: lib/Driver/Driver.cpp:2355
@@ +2354,3 @@
+
+    // Add pch if needed: "If you do not specify an extension as part of the
+    // path name, an extension of .pch is assumed. "
----------------
Maybe add .pch if needed?

================
Comment at: lib/Driver/Tools.cpp:395
@@ +394,3 @@
+    for (const Arg *A : Args.filtered(options::OPT_clang_i_Group)) {
+      // Walk the whole i_Group and the skip non "-include" flags so that the
+      // index here matches the index in the next loop below.
----------------
"the skip" -> just "skip"?

================
Comment at: lib/Driver/Tools.cpp:431
@@ +430,3 @@
+          if (AI >= YcIndex)
+            continue;
+        } else {
----------------
I got confused here, because even if we don't "continue", we still won't hit the else if (A->getOption().matches(options::OPT_include)) branch below.

But that branch is just dealing with implicit includes, we will still do the "Not translated, render as usual." step.

Maybe add a comment to the "else if" branch below that that code is for non-CL style PCH includes?


http://reviews.llvm.org/D17695