[PATCH] D143446: [clang][deps] Ensure module invocation can be serialized

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Having this check is great! Left some nits in-line.



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:644-650
 // May perform round-trip of command line arguments. By default, the round-trip
 // is enabled in assert builds. This can be overwritten at run-time via the
 // "-round-trip-args" and "-no-round-trip-args" command line flags.
 // During round-trip, the command line arguments are parsed into a dummy
 // instance of CompilerInvocation which is used to generate the command line
 // arguments again. The real CompilerInvocation instance is then created by
 // parsing the generated arguments, not the original ones.
----------------
The logic is getting a bit complicated here. I think it'd be good to break down what the `CheckAgainstOriginalInvocation` flag does. Feel free to come up with your own description.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:746
 
-  // Generate arguments again, this time from the options we will end up using
-  // for the rest of the compilation.
   SmallVector<const char *> GeneratedArgs2;
+  if (CheckAgainstOriginalInvocation)
----------------
This name is now not accurate (along with the comment below). Can you make sure these still make sense with the new logic?


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:658
+#endif
+  if (RoundTripArgs.getNumOccurrences() ? RoundTripArgs : DoRoundTripDefault)
+    if (FD.roundTripCommands(llvm::errs()))
----------------
The call to `getNumOccurrences()` is a bit odd. Can you drop that if you add `llvm::cl::init(false)` to the option definition?


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

https://reviews.llvm.org/D143446