[PATCH] D156436: [llvm/OptTable] Print options without documentation

Sun Aug 6 07:43:55 PDT 2023

awarzynski added a comment.

Thanks for the reply and apologies for the delay.

In D156436#4542942 <https://reviews.llvm.org/D156436#4542942>, @j0le wrote:

> In D156436#4542676 <https://reviews.llvm.org/D156436#4542676>, @awarzynski wrote:
>
>> @j0le, thanks for caring about `--help` - it could really benefit from a bit of love. However, I'm not convinced that this is the right direction:
>>
>> [...]
>>
>> We should focus on quality rather than the quantity of the help text being printed. 
>> Personally, I find `clang --help` of little help because it is just a very long dump of unfiltered/uncategorized text. As pointed out in my inline comment, it's perfectly fine to skip some options from the `--help` dump if they don't really contribute any new information. However, categorizing options so that `clang --help` would greatly improve the user experience. At least for Clang. In the case of Flang the list of supported options is still very short and `--help` is quite "manageable".
>
> Yeah, I would also find it better, if we grouped the options some how. Maybe I will find some time for that some day, because this a little bit more work than this small patch.

This would be a non-trivial task - you'd need to "properly" categorise all options in Options.td. But, IMHO, that would be hugely beneficial to the community.

> But I wanna say: The user might be fooled and think the output of "--help" is complete, or identical to https://clang.llvm.org/docs/ClangCommandLineReference.html, but that's not the case.

Yes, but I wouldn't put `-f<option` and `-fno-<option>` into this category. Folks usually know that often both forms are possible. As for `-static` specifically, you could simply add a HelpText. In fact, your patch should allow you to identify all options for which a HelpText is missing.

> I'm fine with discarding this patch,

That would be my suggestion. I really appreciate the effort and, in general, Clang/Flang would greatly benefit from somebody taking greater care of Options.td. However, I don't believe that this patch is the direction that we should be taking. At least not without some additional refactoring that would take care of duplication in the `--help` output (printing `-fbackslash` and `-fno-backslash` is, IMHO, duplication).

If you do decide to purse this further, I will happily review your TableGen changes.

================
Comment at: flang/test/Driver/driver-help-hidden.f90:51
+! CHECK-NEXT: -fno-automatic          Implies the SAVE attribute for non-automatic local objects in subprograms unless RECURSIVE
+! CHECK-NEXT: -fno-backslash
 ! CHECK-NEXT: -fno-color-diagnostics  Disable colors in diagnostics
----------------
j0le wrote:
> awarzynski wrote:
> > IMHO, printing (no help text for the `-fno-<option>` variant):
> > ```
> > -fno-backslash 
> > ```
> > on top of:
> > ```
> >  -fbackslash   Specify that backslash in string introduces an escape character
> > ```
> > is of little value. To me this is noise that we should avoid. If both `-fbackslash ` and `-fno-backslash ` are printed, then both should contain "help" text.
> I agree, it is of little value. But the value is, that the user now knows, that she/he/they/... can toggle of the `-fbackslash` option explicitly. Maybe some wrapper around flang (like a build system), that the user has little control of, sets the option `-fbackslash`, then the user can turn it of with `-fno-backslash`.
> 
> But if we realy want to hide `-fno-backslash` and other `-fno-<option>` options, but still show options that have no help text (but don't have a related negated option), we can come up with some ideas.
> 
> But first let's analyze the implementation: 
> If I understand this correctly, the flang options also come from the clang source tree, i.e. from the file "clang/include/clang/Driver/Options.td".
> 
> Both `-fbackslash` and `-fno-backslash` are defined with this line:
> ```
> defm backslash : OptInFC1FFlag<"backslash", "Specify that backslash in string introduces an escape character">;
> ```
> 
> That corresponds to the following lines in the generated "Options.inc" file:
> ```
> OPTION(prefix_1, llvm::StringLiteral("fbackslash"), fbackslash, Flag, f_Group, INVALID, nullptr, FC1Option | FlangOption | FlangOnlyOption, 0, "Specify that backslash in string introduces an escape character", nullptr, nullptr)
> OPTION(prefix_1, llvm::StringLiteral("fno-backslash"), fno_backslash, Flag, f_Group, INVALID, nullptr, FC1Option | FlangOption | FlangOnlyOption, 0, "", nullptr, nullptr)
> ```
> 
> I don't totally understand how this table generation works. And I don't know how we can tell (programmatically) from this table that `fbackslash` and `fno_backslash` are related. But maybe there is a way, or we have to change the table generation.
> 
> Possible solutions:
> 1. For `OptInFC1FFlag` the "-fno-..." options are marked as hidden.
> 2. We detect in `OptTable::printHelp` that those options are related and group them together
> 3. A help text for the "-fno-..." option is generated from help text of "-f..." option. Either in table generation or in C++ code.
> But the value is, that the user now knows, that she/he/they/... can toggle of the -fbackslash option explicitly. 

But to document that, do we really need:
```
-fno-backslash
-fbackslash   Specify that backslash in string introduces an escape character
```
? Why not something like this:
```
-fbackslash   Specify that backslash in string introduces an escape character (default)
```
And then, somewhere in the documentation:
```
Options with default values (marked with `(default)` at the end of their help text) support both positive (`-f<option>`) and negative (`-fno-<option>`) versions.
```
Or something similar. 

With things like this it's often very helpful to see what GNU do (https://gcc.gnu.org/onlinedocs/gcc/Invoking-GCC.html):
```
Many options have long names starting with ‘-f’ or with ‘-W’—for example, -fmove-loop-invariants, -Wformat and so on. Most of these have both positive and negative forms; the negative form of -ffoo is -fno-foo. This manual documents only one of these two forms, whichever one is not the default. 
```

Following GNU would the path of least surprises :)

>  If I understand this correctly, the flang options also come from the clang source tree, i.e. from the file "clang/include/clang/Driver/Options.td".

Yes, Flang's driver is implemented in terms of Clang's `clangDriver` library and these options are defined within that library. Historically, only Clang would use it. However, with the introduction of Flang, that library has gained a new client - Flang's compiler driver :) This helps us create more consistence experience for end users (there are many other benefits too). Note that the long term plan is to extract the driver library out of Clang. 

> I don't totally understand how this table generation works. 

It's a bit tricky, yes :) Have you checked whether this is a problem specific to Flang options? I would imagine that your change would affect Clang options in a similar way. It just happens that for Flang it's more visible as there's fewer options.

> Possible solutions:

Like hinted above, I suggest that in cases like this, instead of printing two options:
```
-fno-backslash <potentially some help text>
-fbackslash   Specify that backslash in string introduces an escape character
```
Clang and Flang print only one:
```
-fbackslash   Specify that backslash in string introduces an escape character (perhaps some extra marker here?)
```

As for your suggestions:
* Option 1. makes sense to me, though it would pollute `-help-hidden`. You could, though, use some other flag instead of [[ https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/llvm/include/llvm/Option/OptParser.td#L59-L62 | HelpHidden ]].
* Option 3 means duplication that I would rather avoid. 
* Not sure about Option 2 without seeing a prototype/PoC.

> And I don't know how we can tell (programmatically) from this table that fbackslash and fno_backslash are related.
I don't believe they are.

> But maybe there is a way, or we have to change the table generation.

There's always a way :) I think that in this case it would require a bit of design work, but nothing too extensive.

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

https://reviews.llvm.org/D156436