[PATCH] D155729: [OptTable] Make explicitly included options override excluded ones

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Thank you! The clean up is nice. I missed the original change that added the 
DXCOption complexity..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155729

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155729: [OptTable] Make explicitly included options override excluded ones

[Justin Bogner via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb2eda85f047f: [OptTable] Make explicitly included options 
override excluded ones (authored by bogner).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155729

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang/include/clang/Driver/Options.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
  llvm/lib/Option/OptTable.cpp

Index: llvm/lib/Option/OptTable.cpp
===
--- llvm/lib/Option/OptTable.cpp
+++ llvm/lib/Option/OptTable.cpp
@@ -421,7 +421,8 @@
 if (FlagsToInclude && !Opt.hasFlag(FlagsToInclude))
   continue;
 if (Opt.hasFlag(FlagsToExclude))
-  continue;
+  if (!FlagsToInclude || !Opt.hasFlag(FlagsToInclude))
+continue;
 
 // See if this option matches.
 if (std::unique_ptr A =
@@ -650,7 +651,8 @@
 if (FlagsToInclude && !(Flags & FlagsToInclude))
   continue;
 if (Flags & FlagsToExclude)
-  continue;
+  if (!FlagsToInclude || !(Flags & FlagsToInclude))
+continue;
 
 // If an alias doesn't have a help text, show a help text for the aliased
 // option instead.
Index: clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
===
--- clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
+++ clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
@@ -164,8 +164,8 @@
   const unsigned OldPos = Pos;
   std::unique_ptr Arg(OptTable.ParseOneArg(
   ArgList, Pos,
-  /* Include */ ClangCLMode ? CoreOption | CLOption | CLDXCOption : 0,
-  /* Exclude */ ClangCLMode ? 0 : CLOption | CLDXCOption));
+  /* Include */ ClangCLMode ? CoreOption | CLOption : 0,
+  /* Exclude */ ClangCLMode ? 0 : CLOption));
 
   if (!Arg)
 continue;
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6496,7 +6496,6 @@
   if (IsClCompatMode) {
 // Include CL and Core options.
 IncludedFlagsBitmask |= options::CLOption;
-IncludedFlagsBitmask |= options::CLDXCOption;
 IncludedFlagsBitmask |= options::CoreOption;
   } else {
 ExcludedFlagsBitmask |= options::CLOption;
@@ -6504,13 +6503,10 @@
   if (IsDXCMode()) {
 // Include DXC and Core options.
 IncludedFlagsBitmask |= options::DXCOption;
-IncludedFlagsBitmask |= options::CLDXCOption;
 IncludedFlagsBitmask |= options::CoreOption;
   } else {
 ExcludedFlagsBitmask |= options::DXCOption;
   }
-  if (!IsClCompatMode && !IsDXCMode())
-ExcludedFlagsBitmask |= options::CLDXCOption;
 
   return std::make_pair(IncludedFlagsBitmask, ExcludedFlagsBitmask);
 }
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -53,10 +53,6 @@
 // are made available when the driver is running in DXC compatibility mode.
 def DXCOption : OptionFlag;
 
-// CLDXCOption - This is a cl.exe/dxc.exe compatibility option. Options with this flag
-// are made available when the driver is running in CL/DXC compatibility mode.
-def CLDXCOption : OptionFlag;
-
 // NoDriverOption - This option should not be accepted by the driver.
 def NoDriverOption : OptionFlag;
 
@@ -6856,7 +6852,7 @@
 // clang-cl Options
 //===--===//
 
-def cl_Group : OptionGroup<"">, Flags<[CLDXCOption]>,
+def cl_Group : OptionGroup<"">,
   HelpText<"CL.EXE COMPATIBILITY OPTIONS">;
 
 def cl_compile_Group : OptionGroup<"">,
@@ -6865,42 +6861,45 @@
 def cl_ignored_Group : OptionGroup<"">,
   Group;
 
-class CLFlag : Option<["/", "-"], name, KIND_FLAG>,
-  Group, Flags<[CLOption, NoXarchOption]>;
-
-class CLDXCFlag : Option<["/", "-"], name, KIND_FLAG>,
-  Group, Flags<[CLDXCOption, NoXarchOption]>;
-
-class CLCompileFlag : Option<["/", "-"], name, KIND_FLAG>,
-  Group, Flags<[CLOption, NoXarchOption]>;
+class CLFlagOpts flags>
+  : Flags;
 
-class CLIgnoredFlag : Option<["/", "-"], name, KIND_FLAG>,
-  Group, Flags<[CLOption, NoXarchOption]>;
+class CLFlag flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_FLAG>,
+Group, CLFlagOpts;
 
-class CLJoined : Option<["/", "-"], name, KIND_JOINED>,
-  Group, Flags<[CLOption, NoXarchOption]>;
+class CLCompileFlag flags = [CLOption]>
+  : Option<["/", "-"], name, KIND_FLAG>,
+Group, CLFlagOpts;
 
-class CLDXCJoined : Option<["/", "-"], name, KIND_JOINED>,
-  Group, Flags<[CLDXCOption, NoXarchOption]>;
+class CLIgnoredFlag flags = [CLOption]>
+  : Option<["/", "-"], name, 

[PATCH] D155729: [OptTable] Make explicitly included options override excluded ones

[Justin Bogner via Phabricator via cfe-commits]

bogner added inline comments.



Comment at: clang/include/clang/Driver/Options.h:40
+  Ignored = (1 << 18),
+  TargetSpecific = (1 << 19),
 };

bob80905 wrote:
> Given that the id for these flags have changed, does it make sense to write a 
> test that makes sure these flags with these bits still behave as expected? 
> (Ignored and target specific, specifically).
The actual values of the flags aren't really visible anywhere (and in the case 
of ignored, it's actually just changing the value back to what it was before 
https://reviews.llvm.org/D128462). I'm not sure that there's much we can do to 
write such a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155729

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155729: [OptTable] Make explicitly included options override excluded ones

[Joshua Batista via Phabricator via cfe-commits]

bob80905 added inline comments.



Comment at: clang/include/clang/Driver/Options.h:40
+  Ignored = (1 << 18),
+  TargetSpecific = (1 << 19),
 };

Given that the id for these flags have changed, does it make sense to write a 
test that makes sure these flags with these bits still behave as expected? 
(Ignored and target specific, specifically).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155729

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155729: [OptTable] Make explicitly included options override excluded ones

[Xiang Li via Phabricator via cfe-commits]

python3kgae accepted this revision.
python3kgae added a comment.
This revision is now accepted and ready to land.

This is much cleaner.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155729

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155729: [OptTable] Make explicitly included options override excluded ones

[Justin Bogner via Phabricator via cfe-commits]

bogner created this revision.
bogner added reviewers: beanz, kadircet, python3kgae, hans.
Herald added subscribers: arphaman, hiraditya, mcrosier.
Herald added a project: All.
bogner requested review of this revision.
Herald added subscribers: cfe-commits, llvm-commits, MaskRay.
Herald added projects: clang, LLVM, clang-tools-extra.

When we have both explicitly included and excluded option sets, we
were excluding anything from the latter set regardless of what was in
the former. This doesn't compose well and led to an overly complicated
design around DXC options where a third flag was introduced to handle
options that overlapped between DXC and CL.

With this change we check the included options before excluding
anything from the exclude list, which allows for options that are in
multiple categories to be handled in a sensible way. This allows us to
remove CLDXCOption but should otherwise be NFC.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155729

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang/include/clang/Driver/Options.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
  llvm/lib/Option/OptTable.cpp

Index: llvm/lib/Option/OptTable.cpp
===
--- llvm/lib/Option/OptTable.cpp
+++ llvm/lib/Option/OptTable.cpp
@@ -421,7 +421,8 @@
 if (FlagsToInclude && !Opt.hasFlag(FlagsToInclude))
   continue;
 if (Opt.hasFlag(FlagsToExclude))
-  continue;
+  if (!FlagsToInclude || !Opt.hasFlag(FlagsToInclude))
+continue;
 
 // See if this option matches.
 if (std::unique_ptr A =
@@ -650,7 +651,8 @@
 if (FlagsToInclude && !(Flags & FlagsToInclude))
   continue;
 if (Flags & FlagsToExclude)
-  continue;
+  if (!FlagsToInclude || !(Flags & FlagsToInclude))
+continue;
 
 // If an alias doesn't have a help text, show a help text for the aliased
 // option instead.
Index: clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
===
--- clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
+++ clang/lib/Tooling/InterpolatingCompilationDatabase.cpp
@@ -164,8 +164,8 @@
   const unsigned OldPos = Pos;
   std::unique_ptr Arg(OptTable.ParseOneArg(
   ArgList, Pos,
-  /* Include */ ClangCLMode ? CoreOption | CLOption | CLDXCOption : 0,
-  /* Exclude */ ClangCLMode ? 0 : CLOption | CLDXCOption));
+  /* Include */ ClangCLMode ? CoreOption | CLOption : 0,
+  /* Exclude */ ClangCLMode ? 0 : CLOption));
 
   if (!Arg)
 continue;
Index: clang/lib/Driver/Driver.cpp
===
--- clang/lib/Driver/Driver.cpp
+++ clang/lib/Driver/Driver.cpp
@@ -6482,7 +6482,6 @@
   if (IsClCompatMode) {
 // Include CL and Core options.
 IncludedFlagsBitmask |= options::CLOption;
-IncludedFlagsBitmask |= options::CLDXCOption;
 IncludedFlagsBitmask |= options::CoreOption;
   } else {
 ExcludedFlagsBitmask |= options::CLOption;
@@ -6490,13 +6489,10 @@
   if (IsDXCMode()) {
 // Include DXC and Core options.
 IncludedFlagsBitmask |= options::DXCOption;
-IncludedFlagsBitmask |= options::CLDXCOption;
 IncludedFlagsBitmask |= options::CoreOption;
   } else {
 ExcludedFlagsBitmask |= options::DXCOption;
   }
-  if (!IsClCompatMode && !IsDXCMode())
-ExcludedFlagsBitmask |= options::CLDXCOption;
 
   return std::make_pair(IncludedFlagsBitmask, ExcludedFlagsBitmask);
 }
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -53,10 +53,6 @@
 // are made available when the driver is running in DXC compatibility mode.
 def DXCOption : OptionFlag;
 
-// CLDXCOption - This is a cl.exe/dxc.exe compatibility option. Options with this flag
-// are made available when the driver is running in CL/DXC compatibility mode.
-def CLDXCOption : OptionFlag;
-
 // NoDriverOption - This option should not be accepted by the driver.
 def NoDriverOption : OptionFlag;
 
@@ -6844,7 +6840,7 @@
 // clang-cl Options
 //===--===//
 
-def cl_Group : OptionGroup<"">, Flags<[CLDXCOption]>,
+def cl_Group : OptionGroup<"">,
   HelpText<"CL.EXE COMPATIBILITY OPTIONS">;
 
 def cl_compile_Group : OptionGroup<"">,
@@ -6853,42 +6849,45 @@
 def cl_ignored_Group : OptionGroup<"">,
   Group;
 
-class CLFlag : Option<["/", "-"], name, KIND_FLAG>,
-  Group, Flags<[CLOption, NoXarchOption]>;
-
-class CLDXCFlag : Option<["/", "-"], name, KIND_FLAG>,
-  Group, Flags<[CLDXCOption, NoXarchOption]>;
-
-class CLCompileFlag : Option<["/", "-"], name, KIND_FLAG>,
-  Group, Flags<[CLOption, NoXarchOption]>;
+class CLFlagOpts flags>
+  : Flags;