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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3647
+                                         DiagnosticsEngine &Diags) {
+#define OPTION_WITH_MARSHALLING
+#define OPTION_WITH_MARSHALLING_FLAG(PREFIX_TYPE, NAME, ID, KIND, GROUP,       \
----------------
dang wrote:
> dexonsmith wrote:
> > Seems like `Options.inc` could provide this as a default definition, not sure if that seems error-prone?
> Not sure how much of this do you mean? `OPTION_WITH_MARSHALLING` is a convenience thing that forces users to opt into getting the marshalling definitions. I think it might be better to provide default empty definitions for `OPTION_WITH_MARSHALLING_FLAG` and friends to achieve a similar effect without needing `OPTION_WITH_MARSHALLING`. I you mean the actual definitions here they rely on the `ArgList &` to be named Args which might make it error prone to include these as defaults.
Right, I just mean you shouldn't have to define `OPTION_WITH_MARSHALLING` here.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3665-3667
+#undef OPTION_WITH_MARSHALLING_STRING
+#undef OPTION_WITH_MARSHALLING_FLAG
+#undef OPTION_WITH_MARSHALLING
----------------
dang wrote:
> dexonsmith wrote:
> > I prefer the style where the `.inc` file is responsible for the `#undef` calls. It avoids having to duplicate it everywhere (and the risk of forgetting it). WDYT?
> I prefer it too, but the current tablegen backend doesn't generate them for other macros it defines, so I wanted to make it consistent... I could change the existing backend to work that way but I would then need to track all the usages of this across all the llvm-projects which I don't fancy doing. Let me know if you thing it is fine to break with the existing style for this one.
It's probably pretty easy to clean up the existing ones, just grep for `#undef` of any of the defined macros and delete the lines. But if you don't have time I'm fine with you leaving it as-is (i.e. it seems good to be consistent with the rest of the backend).


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

https://reviews.llvm.org/D79796