[PATCH] D105881: [flang][driver] Switch to `BoolFOption` for boolean options

Mon Jul 26 09:50:28 PDT 2021

jansvoboda11 added a comment.

Sorry, I'm not sure I follow.

In D105881#2890134 <https://reviews.llvm.org/D105881#2890134>, @awarzynski wrote:

> Apologies, it has taken me a bit longer to get back to this. @jansvoboda11 , now I'm realising the key disadvantage of using `OptInFFlag/OptOutFFlag` - it's impossible to express the `opt-in`/`opt-out` semantics in TableGen.

What do you mean exactly? I think the semantics are expressed pretty well from Clang's point of view:

- `OptInFFlag` -> users can opt-in into the behavior by specifying the positive flag in `clang -cc1` (e.g. `-fmodules`),
- `OptOutFFlag` -> users can opt-out of the behavior by specifying the negative flag in `clang -cc1` (e.g. `-fno-rtti`).

> In fact, only the contents of `clang -cc1 --help` are being tweaked by using `OptInFFlag/OptOutFFlag`.

That's incorrect, `OptInFFlag` and `OptOutFFlag` both set `HelpText` for the positive and negative driver flags.

> Basically:
>
> - for `OptInFFlang`, only `-ffoo` is printed with `-clang -cc1 --help` (the help text for `-ffno-foo` becomes irrelevant)
> - for `OptOutFFlang`, only `-fno-foo` is printed with `-clang -cc1 --help` (the help text for `-ffoo` is irrelevant)
>
> IIUC, this is achieved through `flags` (`CC1Option` vs `[]`).

Correct.

> In Flang, we rely on the fact that "no help text? don't include it in `flang-new -fc1 --help`" instead. This behavior is implemented in `clangDriver` and applies to all drivers that use it (so this is nothing specific to Flang).

I see. Is this intentional?

> This way, we can always mark Flang options with the relevant flags (e.g. `FC1Option`). In other words, we can claim them and make it clear that other drivers can ignore them.

I don't understand this. Can you elaborate? Are you relying on the absence/presence of a help text to mark flags with `FC1Option`? When does this happen? (In `Options.td`? The TableGen backend? The consumer of `Options.inc`?)

> In general, I think that Options.td would become a bit cleaner and easier to reason about if all options specified all their flags. That wouldn't be easy to achieve though!

I think that the way we define flags for the Clang driver and `-cc1` in one go is already bit confusing. But it's convenient, so we roll with it. On top of that, we have the marshalling infrastructure, that explicitly only applies to the `-cc1` side of things. If we introduce `EmptyKPM` to the mix and say "Marshalling only applies to `-cc1`, but sometimes not at all", things stop making sense in my opinion. People will copy-paste it and be confused things don't work.

> I'm just about send an updated patch. It takes into account the points that I raised above ^^^. It also fixes the failing tests reported by @PeteSteinfeld. Does this new approach work for Clang (I quickly tested `clang -cc1 --help` and see no difference)?

I'd need to double-check if we particularly care about showing/hiding flags without help text.

> It would be nicer to have the `opt-in`/`opt-out` semantics for Flang in TableGen through something similar to `BoolFOption`. No need for this just now - one step at a time :)

I suggest to find another means of achieving the semantics you want. You always just copy the `BoolFOption` implementation and remove the marshalling stuff, if that's really the semantics you want.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105881