dexonsmith added a comment.
I think `DefaultsTo{True,False}` and `InvertedBy{Negative,Positive}Flag` makes
the axes clear, it's way better; thanks for thinking this through.
Thinking out loud: I'm still a bit resistant to making this keypath-centric.
The reason is that `llvm::Option` is a library for command-line options, where
command-options are declared, and the "keypath" feature is optional; changing
the core concept in clang's usage of `llvm::Option` feels incongruent somehow
(especially since clang is the main client). On the other hand (in favour of
being keypath-centric), we're bundling multiple command-line options into a
single conceptual declaration, doing doing one declaration per keypath. I guess
I'm okay with the new semantics. Maybe part of what makes it work is that we're
not declaring a "flag" anymore.
That said, I think you can go further:
In D83892#2425437 <https://reviews.llvm.org/D83892#2425437>, @jansvoboda11
wrote:
> defm inline_line_tables : BoolGOption<DefaultsToFalse,
> InvertedByNegativeFlag,
> "inline-line-tables", "CodeGenOpts.NoInlineLineTables", PositiveFlag<>,
> NegativeFlag<[CC1Option], "Don't emit inline line tables">,
> BothFlags<[CoreOption]>>;
If this is keypath-centric, then a better name for the declaration would be
`no_inline_line_tables`, which matches the keypath. This gives:
defm no_inline_line_tables : BoolOption<DefaultsToFalse,
InvertedByNegativeFlag,
"inline-line-tables", "CodeGenOpts.NoInlineLineTables", PositiveFlag<>,
NegativeFlag<[CC1Option], "Don't emit inline line tables">,
BothFlags<[CoreOption]>>;
... which doesn't seem quite right yet, since the keypath isn't front and
centre.
Here's an idea that's a bit more keypath-centric, that connects the polarity of
the option with the option spelling, and that avoids some of the repetitive
clutter (I added `-something` to show the positive case):
defm no_inline_line_tables : BoolOption<"CodeGenOpts.NoInlineLineTables",
DefaultsToFalse,
ChangedByFlag_No<"inline-line-tables", [CC1Option, CoreOption],
"Don't emit inline line tables">,
CancelFlag<[CoreOption]>>;
defm something : BoolOption<"CodeGenOpts.Something", DefaultsToFalse,
ChangedByFlag<"something", [CC1Option,CoreOption], "Do something">,
CancelFlag<[CoreOption]>>;
This is similar, but might be easier to implement:
defm no_inline_line_tables : BoolOption<"CodeGenOpts.NoInlineLineTables",
DefaultsToFalse,
ChangedBy_No<"inline-line-tables", [CC1Option, CoreOption], [CoreOption],
"Don't emit inline line tables">>;
defm something : BoolOption<"CodeGenOpts.Something", DefaultsToFalse,
ChangedBy<"something", [CC1Option,CoreOption], [CoreOption],
"Do something">>;
I think both of these syntaxes allow you to think primarily about the keypath
and the matching flag.
WDYT?
================
Comment at: clang/include/clang/Driver/Options.td:268
+// (Autolink == true) and disabled by -fno-autolink (Autolink == false).
+// todo: add ImpliedByAnyOf if necessary
+multiclass OptOutPositiveFFlag<string name, string pos_prefix, string
neg_prefix,
----------------
Since you're planning to leave this to-do behind, I have a few nits:
- I think it should be capitalized (`TODO:`) since I feel like I've seen that
more often here.
- It should probably be its own paragraph.
- Complete sentences are a bit nicer (start with a capital and end with a
period).
- The wording "if necessary" suggests we don't know if we'll need to do this,
in which case having a to-do would just be adding noise. But as you mentioned
we do know. I think it's better to be clear about it.
```
// TODO: Add support for ImpliedByAnyOf once it can be
// tested with an option that depends on it.
```
================
Comment at: clang/include/clang/Driver/Options.td:293
MarshallingInfoFlag<keypath, "false">,
ImpliedByAnyOf<disablers, "true">;
}
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > Should the new `OptOutPositiveFFlag` have disablers as well?
> Probably yes, it's mentioned in a todo above. I wanted to add this only when
> an option needs it, to minimize untested code paths.
Ah, thanks, missed that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83892/new/
https://reviews.llvm.org/D83892
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
