tra added inline comments.
================ Comment at: clang/include/clang/Driver/Options.td:1904-1907 +def foffload_lto : Flag<["-"], "foffload-lto">, Flags<[CoreOption]>, Group<f_Group>, + HelpText<"Enable LTO in 'full' mode for offload compilation">; +def fno_offload_lto : Flag<["-"], "fno-offload-lto">, Flags<[CoreOption]>, Group<f_Group>, + HelpText<"Disable LTO mode (default) for offload compilation">; ---------------- yaxunl wrote: > jansvoboda11 wrote: > > yaxunl wrote: > > > tra wrote: > > > > Should it be `BoolFOption` ? > > > Yes. will fix > > The `BoolFOption` marshalling multiclass should be only used for flags > > where either the positive or the negative (or both) are -cc1 options and > > map to a field in `CompilerInvocation`. > > > > Since this patch only deals with the driver (not the cc1 frontend) using > > `BoolFOption` is not correct. Please, revert this change to the previous > > state. > > > > I might need to explicitly call this out in the documentation > > https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option. > will do <off-topic for the patch> @jansvoboda11 Thank you for the explanation. Updating the docs would indeed be useful. I would also suggest describing the restrictions next to the `BoolFOption` definition. Developers tend to read the sources way more often than the docs, and the comments source code make it look like a general-purpose multiclass for any boolean `-f` option. Would it also be possible to add some sort of compile-time safeguards to enforce intended constraints on the CLI tablegen? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99683/new/ https://reviews.llvm.org/D99683 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
