[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I've tested this locally and can confirm that the behavior of `clang` and `flang-new` has been preserved (as in, these changes won't be visible to the end users). Nice!

I think that it would be good to replace `Default` with e.g.

- `Clang`, or
- `ClangDriver`, or
- `ClangCompilerDriver`.

Or, at least, to make the meaning of `Default` much clearer (through e.g. comments). In general, I feel that `Default` is skewed towards this notion that `clang` (the compiler driver tool) is the main client of `clangDriver`. That used to be the case, but with Flang's driver also implemented in terms of `clangDriver` , that's no longer the case. Also, the long term goal is to extract the driver library out of Clang. One day :)

Also, note that `Default` options will be available in both `clang` and `flang-new`. That's the case today (so not something affected by your changes). Ideally, Flang should be able to disable those _Clang options_. That's not possible ATM. Contrary to Flang, Clang can disable _Flang options_ with `FlangOnlyOption`. There is no such flag for Flang ATM, but I think that we could re-use `Default` for that. WDYT?



================
Comment at: clang/docs/InternalsManual.rst:669-670
+  can affect how the option is treated or displayed.
+* ``Vis`` may contain visibility-specific "tags" associated with the option.
+  This lets us filter options for specific tools.
 * ``Alias`` denotes that the option is an alias of another option. This may be
----------------
IMHO, the difference between `Flags` and `Vis` is still unclear. Lets take this opportunity to refine this. IIUC:

* `vis` should be used to specify the drivers in which a particular option would be available. This attribute will impact `tool --help`,
* `flags`  can be used to limit the contexts in which a particular option/flag can be used (e.g. `NoXarchOption` or `LinkerOption`).


================
Comment at: clang/docs/InternalsManual.rst:682-685
+New options are recognized by the Clang driver if ``Vis`` is not specified or
+if it contains ``Default``. Options intended for the ``-cc1`` frontend must be
+explicitly marked with the ``CC1Option`` flag. Flags that specify ``CC1Option``
+but not ``Default`` will only be accessible via ``-cc1``.
----------------
"Clang driver" could mean two things:
* [[ https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/lib/Driver/CMakeLists.txt#L17-L102 | clangDriver ]] - the driver library shared between Clang and Flang,
* [[ https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/tools/driver/CMakeLists.txt#L26-L36 | clang ]] - Clang's compiler driver implemented in terms of `clangDriver` library.

I think that it's important to distinguish between the two in this document. In particular, the meaning of `Default` is confusing. Is the the default for the library (`clangDriver`) or the Clang compiler driver binary, `clang`. I believe it's the latter, but this wording suggests the former.

I appreciate that this wording pre-dates your patch (and probably Flang), but I think that it's worth taking this opportunity to refine this.


================
Comment at: llvm/docs/ReleaseNotes.rst:160
+* The ``Flags`` field of ``llvm::opt::Option`` has been split into ``Flags``
+  and ``Visibility`` to simplify option sharing between clang's drivers.
+  Overloads of ``llvm::opt::OptTable`` that use ``FlagsToInclude`` have been
----------------
[nit] Worth clarifying and advertising a bit more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151