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

Thu Aug 10 11:49:54 PDT 2023

awarzynski added a comment.

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.

> introduced in llvm Option

Could you add a link?

================
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),
----------------
What happens to `CoreOption`s? Same for `NoDriverOption`s?

================
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">;
----------------
What's `Default` in `Vis<[Default, CLOption, DXCOption]>,`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151