[PATCH] D83892: [clang][cli] Port CodeGen option flags to new option parsing system

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

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