[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese added inline comments.


================
Comment at: clang/include/clang/Frontend/CompilerInvocation.h:248
+  /// \returns - True if parsing was successful, false otherwise
+  bool parseSimpleArgs(const llvm::opt::ArgList &Args,
+                       DiagnosticsEngine &Diags);
----------------
Is there a reason for this to be a member of `CompilerInvocation`? The rest of the argument parsing functions are static file local functions.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3658
                                      MissingArgCount, IncludedFlagsBitmask);
+
   LangOptions &LangOpts = *Res.getLangOpts();
----------------
Put formatting fixups in a separate commit.


================
Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:41
+TEST_F(CC1CommandLineGenerationTest, CanGenerateCC1CommandLineFlag) {
+  const char *Args[] = {"clang", "-xc++", "-fmodules-strict-context-hash", "-"};
+
----------------
This reminded me that some of the -cc1 arguments are positional such as `-xc++`. I think we'll need custom handling for `INPUT` arguments, although we don't need them (or want them) for the modules use case. Do you have any thoughts on how to handle them? I don't think answering that should block the initial patch though.


================
Comment at: llvm/include/llvm/Option/OptParser.td:153
+}
+class MarshallingFlagRequired<code keypath, bit ispositive, code defaultvalue>
+  : MarshallingFlag<keypath, ispositive, defaultvalue> {
----------------
I'm not sure Required is a great name here. I initially read that as the option was required, not that it should always be emitted.


================
Comment at: llvm/include/llvm/Option/OptParser.td:167-171
+class MarshallingEnum<code keypath, code defaultvalue, list<code> enumvalues>
+  : OptionMarshallingInfo<"enum", keypath> {
+  code DefaultValue = defaultvalue;
+  list<code>EnumValues = enumvalues;
+}
----------------
I noticed that this isn't being used for the enum case you added (`-mrelocation-model`). Do you intend to use it? You should either use it in this patch or remove it until it actually is used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79796