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

[Daniel Grumberg via Phabricator via llvm-commits]

dang marked 4 inline comments as done.
dang 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);
----------------
Bigcheese wrote:
> Is there a reason for this to be a member of `CompilerInvocation`? The rest of the argument parsing functions are static file local functions.
All the keypaths have to be "relative" to `CompilerInvocation` so that different options can be processed uniformly i.e. we don't want to add extra specialization to the table-gen file that indicates if an option is for the analyzer or for codegen e.t.c. Some of the members of `CompilerInvocation` that store options are private, which means that if parseSimpleArgs which precludes it from being an free function with internal linkage. Unless you think all the `CompilerInvocation` members should be public, but that is a different can of worms.


================
Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:41
+TEST_F(CC1CommandLineGenerationTest, CanGenerateCC1CommandLineFlag) {
+  const char *Args[] = {"clang", "-xc++", "-fmodules-strict-context-hash", "-"};
+
----------------
Bigcheese wrote:
> 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.
`-x` flags are no `INPUT` arguments as in it can appear anywhere but will apply to the inputs. The only part of `CompilerInvocation` that uses positional arguments is the input file(s) which is quite small (21 LOC) so I am happy to leave it as is, having a few line to add it at the end of the command line is not a big problem IMO. 
The thing I am more worried about is that the processing for some options depends on other options having already been processed, I was planning to reorder them accordingly in the td file although I am not a fan of doing this, I think it would be better to addd the information using table-gen's DAG support although that would complicated the table-gen backend.


================
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;
+}
----------------
Bigcheese wrote:
> 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.
Yeah, I forgot to remove it, since the normalizer scheme subsumes the functionality.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79796