[PATCH] D94472: [clang][cli] Command line round-trip for HeaderSearch options

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

This is getting close! A few more comments inline.



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:584-589
+static bool Equal(const ArgStringList &A, const ArgStringList &B) {
+  auto StrEq = [](const char *AElem, const char *BElem) {
+    return StringRef(AElem) == StringRef(BElem);
+  };
+  return std::equal(A.begin(), A.end(), B.begin(), B.end(), StrEq);
+}
----------------
I suggest inlining `Equal` and defining the lambda directly in the call to `std::equal`; I think that'll be easier to read than having this up here. (Nice to have it split out fo rcompile-time if `RoundTrip` is templated, but I have a suggestion about that below.)


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:591-596
+static void Print(ArgStringList &Args, const llvm::Twine &Description) {
+  llvm::dbgs() << Description;
+  for (const char *Arg : Args)
+    llvm::dbgs() << " " << Arg;
+  llvm::dbgs() << "\n";
+}
----------------
- I think this would be more clear as a local lambda too.
- I don't think this is the best destination, since `llvm::dbgs()` goes nowhere when `NDEBUG`. Should this go in `errs()`? Or should `-round-trip-args-debug=` take a filename for where to write extra stuff?
- The arguments won't be correctly formatted for copy-paste if there are any characters that should be escaped. Is the logic from `-###` reusable?


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:604-608
+template <typename ParseFn, typename GenerateFn, typename SwapOptsFn>
+static bool RoundTrip(ParseFn &&Parse, GenerateFn &&Generate,
+                      SwapOptsFn &&SwapOpts, CompilerInvocation &Res,
+                      ArgList &OriginalArgs, DiagnosticsEngine &Diags,
+                      StringRef OptsName) {
----------------
Instead of templating this, can we take `llvm::function_ref`s to type-erase the functions?


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:633
+    SwapOpts(Res);
+    return Parse(Res, OriginalArgs, Diags);
+  }
----------------
I suggest double-checking that the second parse also fails, and reporting that as an error somehow.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:654-656
+  InputArgList GeneratedArgs1 =
+      getDriverOptTable().ParseArgs(GeneratedStrings1, MissingArgIndex1,
+                                    MissingArgCount1, options::CC1Option);
----------------
Does this need to be checked for success / error conditions in any way?


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:663
+  SwapOpts(Res);
+  bool Success2 = Parse(Res, GeneratedArgs1, Diags);
+
----------------
Might be better to error here if the second parse fails. We'd want to debug the generated args too.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:684-685
+  if (!GeneratedStringsEqual)
+    llvm::report_fatal_error("Mismatch between arguments generated during "
+                             "round-trip");
+
----------------
Another option vs. report_fatal_error would be to create (fatal) clang error diagnostics. That applies here, and to my previous comments. WDYT? (@Bigcheese, any thoughts?)

Also, I think `-round-trip-args-debug` might not be super discoverable. Maybe we can add a comment in the code where the errors are reported suggesting adding that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94472/new/

https://reviews.llvm.org/D94472