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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
