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

[Justin Bogner via Phabricator via cfe-commits]

bogner added a comment.

In D157151#4577790 <https://reviews.llvm.org/D157151#4577790>, @awarzynski wrote:

> Hey @bogner , I've only skimmed through so far and it's looking great! That Include/Exclude API was not fun to use. What you are proposing here takes Options.td to a much a better place - this is a much needed and long overdue refactor.
>
> There's quite a bit of churn here, so I will need a few days to scan. In the meantime, could you update flang/docs/FlangDriver.md? And in general, please note that this updates (primarily) `clangDriver` logic, which is used by both by Clang and Flang. In particular, Options.td is shared. I think that it's worth highlighting that this change benefits both sub-projects.

Yep, sorry about that. I tried to break this up as much as I could but given the number of places that depend on Options.td it was tricky. I do think this should makes things much easier for flang's driver in particular, as well as making clang-cl and clang-dxc easier to deal with.

Sorry I missed updating FlangDriver.md - FWIW I did build flang and run those tests with this change. I'll update the patch with doc updates momentarily.

>> introduced in llvm Option
>
> Could you add a link?

I've updated the description to mention https://reviews.llvm.org/D157149.



================
Comment at: clang/include/clang/Driver/Options.h:25-37
-  CoreOption = (1 << 8),
-  CLOption = (1 << 9),
-  CC1Option = (1 << 10),
-  CC1AsOption = (1 << 11),
-  NoDriverOption = (1 << 12),
-  LinkOption = (1 << 13),
-  FlangOption = (1 << 14),
----------------
awarzynski wrote:
> What happens to `CoreOption`s? Same for `NoDriverOption`s?
- `CoreOption` meant "available in clang and clang-cl (and maybe dxc...)", so it's now `[Default, CLOption]`.
- `NoDriverOption` is `!Default` - negative flags were a big part of why the include/exclude thing was awkward, so the change in https://reviews.llvm.org/D157149 introduced a bit for options where you don't say anything specific, which meant the clang driver in practice.

For a complete map, see `flag_to_vis` in the `update_options_td_flags.py` helper script I provided for downstreams: https://reviews.llvm.org/D157151#change-FMUYy4yRDQLQ


================
Comment at: clang/include/clang/Driver/Options.td:193
 def m_x86_Features_Group : OptionGroup<"<x86 features group>">,
-                           Group<m_Group>, Flags<[CoreOption]>, DocName<"X86">;
+                           Group<m_Group>, Vis<[Default, CLOption, DXCOption]>,
+                           DocName<"X86">;
----------------
awarzynski wrote:
> What's `Default` in `Vis<[Default, CLOption, DXCOption]>,`?
If you don't specify any visibility, the visibility is "Default" (See https://reviews.llvm.org/D157149). For all intents and purposes this means "Clang Driver" in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151