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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
