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