jansvoboda11 added inline comments.
================
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);
+}
----------------
dexonsmith wrote:
> 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.)
That's fair.
================
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";
+}
----------------
dexonsmith wrote:
> - 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?
Good point, resolved.
================
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) {
----------------
dexonsmith wrote:
> Instead of templating this, can we take `llvm::function_ref`s to type-erase
> the functions?
Yes, that would be better. I had this templated while I was figuring out what
their signature should even be. I've extracted typedefs to keep the signature
of `RoundTrip` readable.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:633
+ SwapOpts(Res);
+ return Parse(Res, OriginalArgs, Diags);
+ }
----------------
dexonsmith wrote:
> I suggest double-checking that the second parse also fails, and reporting
> that as an error somehow.
I think this should never happen, but being defensive here is a good idea.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:654-656
+ InputArgList GeneratedArgs1 =
+ getDriverOptTable().ParseArgs(GeneratedStrings1, MissingArgIndex1,
+ MissingArgCount1, options::CC1Option);
----------------
dexonsmith wrote:
> Does this need to be checked for success / error conditions in any way?
Other callers usually check for missing argument values or unknown arguments.
If we did that, we'd need to distinguish between bad arguments we've copied
from `OriginalArgs` and errors we created in `Generate`. We don't have access
to `MissingArgIndex` and `MissingArgsCount` of `OriginalArgs`, so it's not that
easy. We could serialize and parse `OriginalArgs` again, but I think that's not
a good idea from performance perspective.
I suggest we omit the checks for now, but put them in place as soon as we
round-trip all arguments.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:663
+ SwapOpts(Res);
+ bool Success2 = Parse(Res, GeneratedArgs1, Diags);
+
----------------
dexonsmith wrote:
> Might be better to error here if the second parse fails. We'd want to debug
> the generated args too.
That's right.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:684-685
+ if (!GeneratedStringsEqual)
+ llvm::report_fatal_error("Mismatch between arguments generated during "
+ "round-trip");
+
----------------
dexonsmith wrote:
> 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.
If we decide to go with custom diagnostics instead of just using `llvm::errs`
and `llvm::report_fatal_error`, we'd need to instantiate a custom
`DiagnosticsEngine` here in `RoundTrip`, because we cannot rely on clients
passing us reasonable `Diags`.
One such example is `CompilerInvocationTest`, where we don't care what
diagnostics get emitted (by using `TextDiagnosticBuffer` consumer). The errors
we're emitting here would be still useful to see when testing though.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94472/new/
https://reviews.llvm.org/D94472
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
