[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation
Daniel Grumberg via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 5 04:55:53 PDT 2020
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
More information about the llvm-commits
mailing list