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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
