[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Sorry for taking so long. I created a patch that should fix the missing 
`SourceManager`: D99414 

@rupprecht can you confirm this works for your use-case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Thanks for reporting that, @rupprecht. I'll look into the issue in a couple of 
days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I ran into this commit when integrating commits today (specifically, 
97100646d1b4526de1eac3aacdb0b098739c6ec9 
) -- 
there's nothing wrong with this patch AFAICT, but I'm wondering if the error 
messaging/handling could be improved somehow.

tl;dr I reduced an example in D94468 

We have an internal user creating a ToolInvocation and attaching a 
DiagnosticConsumer to it. They were also using `-ferror-limit=-1`, which if I 
understand now, may be meaningless -- `-ferror-limit=0` is used to mean 
"unlimited error messages", which is probably what they meant.

Anyway, with the DiagnosticConsumer attached, the test crashed with an 
assertion failure `"Assertion 'SourceMgr && "SourceManager not set!"` failed.", 
which really wasn't helpful at all. After much longer than I care to admit, I 
thought to remove the `setDiagnosticConsumer`, and was able to find the 
`"invalid integral value '-1' in '-ferror-limit -1'"` error in the logs, and I 
was able to find the real issue pretty quickly after that.

I don't think the usage was out of the ordinary, so I created D94468 
 as an example if you have time to take a 
look, to save the next person that may run into this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda 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 rG8e3230ffa3ad: [clang][cli] Port DiagnosticOpts to new option 
parsing system (authored by jansvoboda11).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -66,6 +66,7 @@
   static constexpr const char *MacroName = "OPTION_WITH_MARSHALLING";
   const Record 
   bool ShouldAlwaysEmit;
+  StringRef MacroPrefix;
   StringRef KeyPath;
   StringRef DefaultValue;
   StringRef NormalizedValuesScope;
@@ -100,6 +101,10 @@
 
   MarshallingInfo(const Record ) : R(R) {}
 
+  std::string getMacroName() const {
+return (MacroPrefix + MarshallingInfo::MacroName).str();
+  }
+
   void emit(raw_ostream ) const {
 write_cstring(OS, StringRef(getOptionSpelling(R)));
 OS << ", ";
@@ -163,6 +168,7 @@
   MarshallingInfo Ret(R);
 
   Ret.ShouldAlwaysEmit = R.getValueAsBit("ShouldAlwaysEmit");
+  Ret.MacroPrefix = R.getValueAsString("MacroPrefix");
   Ret.KeyPath = R.getValueAsString("KeyPath");
   Ret.DefaultValue = R.getValueAsString("DefaultValue");
   Ret.NormalizedValuesScope = R.getValueAsString("NormalizedValuesScope");
@@ -424,13 +430,13 @@
 MarshallingInfos.push_back(createMarshallingInfo(*R));
 
   for (const auto  : MarshallingInfos) {
-OS << "#ifdef " << MarshallingInfo::MacroName << "\n";
-OS << MarshallingInfo::MacroName << "(";
+OS << "#ifdef " << MI.getMacroName() << "\n";
+OS << MI.getMacroName() << "(";
 WriteOptRecordFields(OS, MI.R);
 OS << ", ";
 MI.emit(OS);
 OS << ")\n";
-OS << "#endif // " << MarshallingInfo::MacroName << "\n";
+OS << "#endif // " << MI.getMacroName() << "\n";
   }
 
   OS << "\n";
Index: llvm/include/llvm/Option/OptParser.td
===
--- llvm/include/llvm/Option/OptParser.td
+++ llvm/include/llvm/Option/OptParser.td
@@ -97,6 +97,7 @@
   OptionGroup Group = ?;
   Option Alias = ?;
   list AliasArgs = [];
+  code MacroPrefix = "";
   code KeyPath = ?;
   code DefaultValue = ?;
   code ImpliedValue = ?;
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -680,4 +680,19 @@
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-mad-enable")));
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-menable-unsafe-fp-math")));
 }
+
+// Diagnostic option.
+
+TEST_F(CommandLineTest, DiagnosticOptionPresent) {
+  const char *Args[] = {"-verify=xyz"};
+
+  ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
+
+  ASSERT_EQ(Invocation.getDiagnosticOpts().VerifyPrefixes,
+std::vector({"xyz"}));
+
+  Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+  ASSERT_THAT(GeneratedArgs, ContainsN(StrEq("-verify=xyz"), 1));
+}
 } // anonymous namespace
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -388,7 +388,6 @@
 DiagnosticsEngine ,
 const InputArgList ) {
   LangOptions  = *Invocation.getLangOpts();
-  DiagnosticOptions  = Invocation.getDiagnosticOpts();
   CodeGenOptions  = Invocation.getCodeGenOpts();
   TargetOptions  = Invocation.getTargetOpts();
   FrontendOptions  = Invocation.getFrontendOpts();
@@ -402,8 +401,6 @@
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
-  llvm::sys::Process::UseANSIEscapeCodes(DiagOpts.UseANSIEscapeCodes);
-
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
@@ -1404,14 +1401,14 @@
   IMPLIED_CHECK, IMPLIED_VALUE,\
   NORMALIZER, MERGER, TABLE_INDEX) \
   if ((FLAGS)::CC1Option) {\
-this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);  \
+KEYPATH = MERGER(KEYPATH, DEFAULT_VALUE);  \
 if (IMPLIED_CHECK) \
-  this->KEYPATH = MERGER(this->KEYPATH, 

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1193-1195
+defm caret_diagnostics : BoolFOption<"caret-diagnostics",
+  "DiagnosticOpts->ShowCarets", DefaultsToTrue,
+  ChangedBy, ResetBy>, IsDiag;

dexonsmith wrote:
> There was one thing in the original patch that was a bit of sanity check for 
> whether `IsDiag` was added to the wrong option: the `DiagnosticOpts->` part 
> of the keypath was implicit. What do you think of adding that back? That 
> would make the keypath here `ShowCarets`.
> 
> It highlights that diagnostics are special (since `DiagnosticOpts` is never 
> mentioned in the file, and any mistakes could be found with a `grep`).
I agree that now, when we don't use the keypath as the "source of truth", the 
original solution where `DiagnosticOpts->` is implied by something else is 
cleaner. Thanks for pointing that out!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 315131.
jansvoboda11 added a comment.

Don't create diagnostics_fixit_info via BoolXxxOption to keep the distinstion 
of Group and Group


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -66,6 +66,7 @@
   static constexpr const char *MacroName = "OPTION_WITH_MARSHALLING";
   const Record 
   bool ShouldAlwaysEmit;
+  StringRef MacroPrefix;
   StringRef KeyPath;
   StringRef DefaultValue;
   StringRef NormalizedValuesScope;
@@ -100,6 +101,10 @@
 
   MarshallingInfo(const Record ) : R(R) {}
 
+  std::string getMacroName() const {
+return (MacroPrefix + MarshallingInfo::MacroName).str();
+  }
+
   void emit(raw_ostream ) const {
 write_cstring(OS, StringRef(getOptionSpelling(R)));
 OS << ", ";
@@ -163,6 +168,7 @@
   MarshallingInfo Ret(R);
 
   Ret.ShouldAlwaysEmit = R.getValueAsBit("ShouldAlwaysEmit");
+  Ret.MacroPrefix = R.getValueAsString("MacroPrefix");
   Ret.KeyPath = R.getValueAsString("KeyPath");
   Ret.DefaultValue = R.getValueAsString("DefaultValue");
   Ret.NormalizedValuesScope = R.getValueAsString("NormalizedValuesScope");
@@ -424,13 +430,13 @@
 MarshallingInfos.push_back(createMarshallingInfo(*R));
 
   for (const auto  : MarshallingInfos) {
-OS << "#ifdef " << MarshallingInfo::MacroName << "\n";
-OS << MarshallingInfo::MacroName << "(";
+OS << "#ifdef " << MI.getMacroName() << "\n";
+OS << MI.getMacroName() << "(";
 WriteOptRecordFields(OS, MI.R);
 OS << ", ";
 MI.emit(OS);
 OS << ")\n";
-OS << "#endif // " << MarshallingInfo::MacroName << "\n";
+OS << "#endif // " << MI.getMacroName() << "\n";
   }
 
   OS << "\n";
Index: llvm/include/llvm/Option/OptParser.td
===
--- llvm/include/llvm/Option/OptParser.td
+++ llvm/include/llvm/Option/OptParser.td
@@ -97,6 +97,7 @@
   OptionGroup Group = ?;
   Option Alias = ?;
   list AliasArgs = [];
+  code MacroPrefix = "";
   code KeyPath = ?;
   code DefaultValue = ?;
   code ImpliedValue = ?;
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -680,4 +680,19 @@
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-mad-enable")));
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-menable-unsafe-fp-math")));
 }
+
+// Diagnostic option.
+
+TEST_F(CommandLineTest, DiagnosticOptionPresent) {
+  const char *Args[] = {"-verify=xyz"};
+
+  ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
+
+  ASSERT_EQ(Invocation.getDiagnosticOpts().VerifyPrefixes,
+std::vector({"xyz"}));
+
+  Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+  ASSERT_THAT(GeneratedArgs, ContainsN(StrEq("-verify=xyz"), 1));
+}
 } // anonymous namespace
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -388,7 +388,6 @@
 DiagnosticsEngine ,
 const InputArgList ) {
   LangOptions  = *Invocation.getLangOpts();
-  DiagnosticOptions  = Invocation.getDiagnosticOpts();
   CodeGenOptions  = Invocation.getCodeGenOpts();
   TargetOptions  = Invocation.getTargetOpts();
   FrontendOptions  = Invocation.getFrontendOpts();
@@ -402,8 +401,6 @@
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
-  llvm::sys::Process::UseANSIEscapeCodes(DiagOpts.UseANSIEscapeCodes);
-
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
@@ -1404,14 +1401,14 @@
   IMPLIED_CHECK, IMPLIED_VALUE,\
   NORMALIZER, MERGER, TABLE_INDEX) \
   if ((FLAGS)::CC1Option) {\
-this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);  \
+KEYPATH = MERGER(KEYPATH, DEFAULT_VALUE);  \
 if (IMPLIED_CHECK) \
-  this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);\
+  KEYPATH = MERGER(KEYPATH, 

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 315117.
jansvoboda11 added a comment.

Move the "DiagnosticOpts->" prefix from TableGen definitions to macros


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -66,6 +66,7 @@
   static constexpr const char *MacroName = "OPTION_WITH_MARSHALLING";
   const Record 
   bool ShouldAlwaysEmit;
+  StringRef MacroPrefix;
   StringRef KeyPath;
   StringRef DefaultValue;
   StringRef NormalizedValuesScope;
@@ -100,6 +101,10 @@
 
   MarshallingInfo(const Record ) : R(R) {}
 
+  std::string getMacroName() const {
+return (MacroPrefix + MarshallingInfo::MacroName).str();
+  }
+
   void emit(raw_ostream ) const {
 write_cstring(OS, StringRef(getOptionSpelling(R)));
 OS << ", ";
@@ -163,6 +168,7 @@
   MarshallingInfo Ret(R);
 
   Ret.ShouldAlwaysEmit = R.getValueAsBit("ShouldAlwaysEmit");
+  Ret.MacroPrefix = R.getValueAsString("MacroPrefix");
   Ret.KeyPath = R.getValueAsString("KeyPath");
   Ret.DefaultValue = R.getValueAsString("DefaultValue");
   Ret.NormalizedValuesScope = R.getValueAsString("NormalizedValuesScope");
@@ -424,13 +430,13 @@
 MarshallingInfos.push_back(createMarshallingInfo(*R));
 
   for (const auto  : MarshallingInfos) {
-OS << "#ifdef " << MarshallingInfo::MacroName << "\n";
-OS << MarshallingInfo::MacroName << "(";
+OS << "#ifdef " << MI.getMacroName() << "\n";
+OS << MI.getMacroName() << "(";
 WriteOptRecordFields(OS, MI.R);
 OS << ", ";
 MI.emit(OS);
 OS << ")\n";
-OS << "#endif // " << MarshallingInfo::MacroName << "\n";
+OS << "#endif // " << MI.getMacroName() << "\n";
   }
 
   OS << "\n";
Index: llvm/include/llvm/Option/OptParser.td
===
--- llvm/include/llvm/Option/OptParser.td
+++ llvm/include/llvm/Option/OptParser.td
@@ -97,6 +97,7 @@
   OptionGroup Group = ?;
   Option Alias = ?;
   list AliasArgs = [];
+  code MacroPrefix = "";
   code KeyPath = ?;
   code DefaultValue = ?;
   code ImpliedValue = ?;
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -680,4 +680,19 @@
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-mad-enable")));
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-menable-unsafe-fp-math")));
 }
+
+// Diagnostic option.
+
+TEST_F(CommandLineTest, DiagnosticOptionPresent) {
+  const char *Args[] = {"-verify=xyz"};
+
+  ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
+
+  ASSERT_EQ(Invocation.getDiagnosticOpts().VerifyPrefixes,
+std::vector({"xyz"}));
+
+  Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+  ASSERT_THAT(GeneratedArgs, ContainsN(StrEq("-verify=xyz"), 1));
+}
 } // anonymous namespace
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -388,7 +388,6 @@
 DiagnosticsEngine ,
 const InputArgList ) {
   LangOptions  = *Invocation.getLangOpts();
-  DiagnosticOptions  = Invocation.getDiagnosticOpts();
   CodeGenOptions  = Invocation.getCodeGenOpts();
   TargetOptions  = Invocation.getTargetOpts();
   FrontendOptions  = Invocation.getFrontendOpts();
@@ -402,8 +401,6 @@
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
-  llvm::sys::Process::UseANSIEscapeCodes(DiagOpts.UseANSIEscapeCodes);
-
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
@@ -1404,14 +1401,13 @@
   IMPLIED_CHECK, IMPLIED_VALUE,\
   NORMALIZER, MERGER, TABLE_INDEX) \
   if ((FLAGS)::CC1Option) {\
-this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);  \
+KEYPATH = MERGER(KEYPATH, DEFAULT_VALUE);  \
 if (IMPLIED_CHECK) \
-  this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);\
+  KEYPATH = MERGER(KEYPATH, IMPLIED_VALUE);  

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 315115.
jansvoboda11 added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -66,6 +66,7 @@
   static constexpr const char *MacroName = "OPTION_WITH_MARSHALLING";
   const Record 
   bool ShouldAlwaysEmit;
+  StringRef MacroPrefix;
   StringRef KeyPath;
   StringRef DefaultValue;
   StringRef NormalizedValuesScope;
@@ -100,6 +101,10 @@
 
   MarshallingInfo(const Record ) : R(R) {}
 
+  std::string getMacroName() const {
+return (MacroPrefix + MarshallingInfo::MacroName).str();
+  }
+
   void emit(raw_ostream ) const {
 write_cstring(OS, StringRef(getOptionSpelling(R)));
 OS << ", ";
@@ -163,6 +168,7 @@
   MarshallingInfo Ret(R);
 
   Ret.ShouldAlwaysEmit = R.getValueAsBit("ShouldAlwaysEmit");
+  Ret.MacroPrefix = R.getValueAsString("MacroPrefix");
   Ret.KeyPath = R.getValueAsString("KeyPath");
   Ret.DefaultValue = R.getValueAsString("DefaultValue");
   Ret.NormalizedValuesScope = R.getValueAsString("NormalizedValuesScope");
@@ -424,13 +430,13 @@
 MarshallingInfos.push_back(createMarshallingInfo(*R));
 
   for (const auto  : MarshallingInfos) {
-OS << "#ifdef " << MarshallingInfo::MacroName << "\n";
-OS << MarshallingInfo::MacroName << "(";
+OS << "#ifdef " << MI.getMacroName() << "\n";
+OS << MI.getMacroName() << "(";
 WriteOptRecordFields(OS, MI.R);
 OS << ", ";
 MI.emit(OS);
 OS << ")\n";
-OS << "#endif // " << MarshallingInfo::MacroName << "\n";
+OS << "#endif // " << MI.getMacroName() << "\n";
   }
 
   OS << "\n";
Index: llvm/include/llvm/Option/OptParser.td
===
--- llvm/include/llvm/Option/OptParser.td
+++ llvm/include/llvm/Option/OptParser.td
@@ -97,6 +97,7 @@
   OptionGroup Group = ?;
   Option Alias = ?;
   list AliasArgs = [];
+  code MacroPrefix = "";
   code KeyPath = ?;
   code DefaultValue = ?;
   code ImpliedValue = ?;
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -680,4 +680,19 @@
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-mad-enable")));
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-menable-unsafe-fp-math")));
 }
+
+// Diagnostic option.
+
+TEST_F(CommandLineTest, DiagnosticOptionPresent) {
+  const char *Args[] = {"-verify=xyz"};
+
+  ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
+
+  ASSERT_EQ(Invocation.getDiagnosticOpts().VerifyPrefixes,
+std::vector({"xyz"}));
+
+  Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+  ASSERT_THAT(GeneratedArgs, ContainsN(StrEq("-verify=xyz"), 1));
+}
 } // anonymous namespace
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -388,7 +388,6 @@
 DiagnosticsEngine ,
 const InputArgList ) {
   LangOptions  = *Invocation.getLangOpts();
-  DiagnosticOptions  = Invocation.getDiagnosticOpts();
   CodeGenOptions  = Invocation.getCodeGenOpts();
   TargetOptions  = Invocation.getTargetOpts();
   FrontendOptions  = Invocation.getFrontendOpts();
@@ -402,8 +401,6 @@
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
-  llvm::sys::Process::UseANSIEscapeCodes(DiagOpts.UseANSIEscapeCodes);
-
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
@@ -1404,14 +1401,13 @@
   IMPLIED_CHECK, IMPLIED_VALUE,\
   NORMALIZER, MERGER, TABLE_INDEX) \
   if ((FLAGS)::CC1Option) {\
-this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);  \
+KEYPATH = MERGER(KEYPATH, DEFAULT_VALUE);  \
 if (IMPLIED_CHECK) \
-  this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);\
+  KEYPATH = MERGER(KEYPATH, IMPLIED_VALUE);\
 if (SHOULD_PARSE)  

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1193-1195
+defm caret_diagnostics : BoolFOption<"caret-diagnostics",
+  "DiagnosticOpts->ShowCarets", DefaultsToTrue,
+  ChangedBy, ResetBy>, IsDiag;

There was one thing in the original patch that was a bit of sanity check for 
whether `IsDiag` was added to the wrong option: the `DiagnosticOpts->` part of 
the keypath was implicit. What do you think of adding that back? That would 
make the keypath here `ShowCarets`.

It highlights that diagnostics are special (since `DiagnosticOpts` is never 
mentioned in the file, and any mistakes could be found with a `grep`).



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3212
+
+#define DIAG_OPTION_WITH_MARSHALLING OPTION_WITH_MARSHALLING
+

Making `DiagnosticOpts->` implicit requires a little more code here:
```
#define DIAG_OPTION_WITH_MARSHALLING(  \
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  \
IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \
TABLE_INDEX)   \
  GENERATE_OPTION_WITH_MARSHALLING(\
  Args, SA, KIND, FLAGS, SPELLING, ALWAYS_EMIT, DiagnosticOpts->KEYPATH,   \
  DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, DENORMALIZER, EXTRACTOR,\
  TABLE_INDEX)
```
But I think it might be worth it for the sanity check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:102-107
+  std::string getMacroName() const {
+if (KeyPath.startswith("DiagnosticOpts."))
+  return (Twine("DIAG_") + MarshallingInfo::MacroName).str();
+
+return MarshallingInfo::MacroName;
+  }

dexonsmith wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > This seems like a bit of a semantic layering violation. It'd be pretty 
> > > unexpected if someone renamed `DiagnosticOpts` in clang that they'd have 
> > > to update this code in llvm. Is there another way to solve this problem?
> > I don't like it either, but the alternatives I can think of are worse.
> > 
> > We could add a `string MacroPrefix;` field to LLVM's `Option` class and 
> > populate it in Clang's TableGen file:
> > 1. Via something like an `IsDiag` multiclass that we'd need to remember to 
> > apply to each diagnostic option. I don't like it as it seems error prone 
> > and introduces duplication.
> > 2. Put all diagnostic options into a single `let MacroPrefix = "DIAG_" in { 
> > ... }` block. This removes the duplication, but doesn't ensure an option is 
> > in that block iff it's a diagnostic option with `"DiagnosticOpts.*"` 
> > keypath.
> > 3. More involved approach would be to duplicate the LLVM's `Option` and 
> > related stuff in Clang. That would get us a place to put the custom 
> > `KeyPath.startswith("DiagnosticOpts.")` logic and then forward to LLVM's 
> > `Option` with the appropriate `MacroPrefix`.
> > 
> > I'll think some more about it.
> Doing #1 + #2 seems like an okay tradeoff to me (looking back at the old 
> diff, it seems like that's similar to what @dang originally implemented 
> (except adding a more generic `MacroPrefix`), and it seems fairly clean / 
> obvious to me).
> 
> > [...] but doesn't ensure an option is in that block iff it's a diagnostic 
> > option with "DiagnosticOpts.*" keypath.
> 
> The reason I'm okay with this is that I'm having trouble envisioning how this 
> would go wrong practice.
> - If someone adds somethings to `DiagnosticOptions`, they're likely to grep 
> for how the adjacent field was defined / marshalled, duplicate the line, and 
> modify it. I'm not seeing a likely path for them to copy/paste from a 
> non-diagnostic option and/or miss adding this to the `let` block.
> - If someone accidentally adds something to the `let` block that isn't in 
> `DiagnosticOptions`, they'll get a compiler error in `ParseDiagnosticArgs`.
> 
> If you're still concerned, I wonder if there's a way to add a check in 
> asserts builds that confirms that `ParseDiagnosticArgs` fills in 
> `DiagnosticOptions` equivalently to how `createFromCommandLine` does? (and/or 
> could the latter call the former as an implementation strategy?)
> - If someone adds somethings to `DiagnosticOptions`, they're likely to grep 
> for how the adjacent field was defined / marshalled, duplicate the line, and 
> modify it. I'm not seeing a likely path for them to copy/paste from a 
> non-diagnostic option and/or miss adding this to the `let` block.

I think that's a fair assumption.

> - If someone accidentally adds something to the `let` block that isn't in 
> `DiagnosticOptions`, they'll get a compiler error in `ParseDiagnosticArgs`.

That's right.

I think it's fine to move from the check in `OptParserEmitter` to a 
`MacroPrefix` then.
I chose the `IsDiag` mixin as opposed to `let MacroPrefix = "_DIAG" in { ... 
}`, as it doesn't require moving options around - cleaner diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 314907.
jansvoboda11 added a comment.

Use arrow instead of dot in keypath


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -66,6 +66,7 @@
   static constexpr const char *MacroName = "OPTION_WITH_MARSHALLING";
   const Record 
   bool ShouldAlwaysEmit;
+  StringRef MacroPrefix;
   StringRef KeyPath;
   StringRef DefaultValue;
   StringRef NormalizedValuesScope;
@@ -99,6 +100,10 @@
 
   MarshallingInfo(const Record ) : R(R) {}
 
+  std::string getMacroName() const {
+return (MacroPrefix + MarshallingInfo::MacroName).str();
+  }
+
   void emit(raw_ostream ) const {
 write_cstring(OS, StringRef(getOptionSpelling(R)));
 OS << ", ";
@@ -160,6 +165,7 @@
   MarshallingInfo Ret(R);
 
   Ret.ShouldAlwaysEmit = R.getValueAsBit("ShouldAlwaysEmit");
+  Ret.MacroPrefix = R.getValueAsString("MacroPrefix");
   Ret.KeyPath = R.getValueAsString("KeyPath");
   Ret.DefaultValue = R.getValueAsString("DefaultValue");
   Ret.NormalizedValuesScope = R.getValueAsString("NormalizedValuesScope");
@@ -420,13 +426,13 @@
 MarshallingInfos.push_back(createMarshallingInfo(*R));
 
   for (const auto  : MarshallingInfos) {
-OS << "#ifdef " << MarshallingInfo::MacroName << "\n";
-OS << MarshallingInfo::MacroName << "(";
+OS << "#ifdef " << MI.getMacroName() << "\n";
+OS << MI.getMacroName() << "(";
 WriteOptRecordFields(OS, MI.R);
 OS << ", ";
 MI.emit(OS);
 OS << ")\n";
-OS << "#endif // " << MarshallingInfo::MacroName << "\n";
+OS << "#endif // " << MI.getMacroName() << "\n";
   }
 
   OS << "\n";
Index: llvm/include/llvm/Option/OptParser.td
===
--- llvm/include/llvm/Option/OptParser.td
+++ llvm/include/llvm/Option/OptParser.td
@@ -97,6 +97,7 @@
   OptionGroup Group = ?;
   Option Alias = ?;
   list AliasArgs = [];
+  code MacroPrefix = "";
   code KeyPath = ?;
   code DefaultValue = ?;
   code ImpliedValue = ?;
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -606,4 +606,19 @@
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-mad-enable")));
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-menable-unsafe-fp-math")));
 }
+
+// Diagnostic option.
+
+TEST_F(CommandLineTest, DiagnosticOptionPresent) {
+  const char *Args[] = {"-verify=xyz"};
+
+  ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
+
+  ASSERT_EQ(Invocation.getDiagnosticOpts().VerifyPrefixes,
+std::vector({"xyz"}));
+
+  Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+  ASSERT_THAT(GeneratedArgs, ContainsN(StrEq("-verify=xyz"), 1));
+}
 } // anonymous namespace
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -386,7 +386,6 @@
 DiagnosticsEngine ,
 const InputArgList ) {
   LangOptions  = *Invocation.getLangOpts();
-  DiagnosticOptions  = Invocation.getDiagnosticOpts();
   CodeGenOptions  = Invocation.getCodeGenOpts();
   TargetOptions  = Invocation.getTargetOpts();
   FrontendOptions  = Invocation.getFrontendOpts();
@@ -400,8 +399,6 @@
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
-  llvm::sys::Process::UseANSIEscapeCodes(DiagOpts.UseANSIEscapeCodes);
-
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
@@ -1401,13 +1398,12 @@
 ARGS, DIAGS, SUCCESS, ID, FLAGS, PARAM, KEYPATH, DEFAULT_VALUE,\
 IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, MERGER, TABLE_INDEX) \
   if ((FLAGS)::CC1Option) {\
-this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);  \
+KEYPATH = MERGER(KEYPATH, DEFAULT_VALUE);  \
 if (IMPLIED_CHECK) \
-  this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);\
+  KEYPATH = MERGER(KEYPATH, IMPLIED_VALUE);\
 if (auto 

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 314900.
jansvoboda11 added a comment.

Introduce `IsDiag` mixin, add test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -66,6 +66,7 @@
   static constexpr const char *MacroName = "OPTION_WITH_MARSHALLING";
   const Record 
   bool ShouldAlwaysEmit;
+  StringRef MacroPrefix;
   StringRef KeyPath;
   StringRef DefaultValue;
   StringRef NormalizedValuesScope;
@@ -99,6 +100,10 @@
 
   MarshallingInfo(const Record ) : R(R) {}
 
+  std::string getMacroName() const {
+return (MacroPrefix + MarshallingInfo::MacroName).str();
+  }
+
   void emit(raw_ostream ) const {
 write_cstring(OS, StringRef(getOptionSpelling(R)));
 OS << ", ";
@@ -160,6 +165,7 @@
   MarshallingInfo Ret(R);
 
   Ret.ShouldAlwaysEmit = R.getValueAsBit("ShouldAlwaysEmit");
+  Ret.MacroPrefix = R.getValueAsString("MacroPrefix");
   Ret.KeyPath = R.getValueAsString("KeyPath");
   Ret.DefaultValue = R.getValueAsString("DefaultValue");
   Ret.NormalizedValuesScope = R.getValueAsString("NormalizedValuesScope");
@@ -420,13 +426,13 @@
 MarshallingInfos.push_back(createMarshallingInfo(*R));
 
   for (const auto  : MarshallingInfos) {
-OS << "#ifdef " << MarshallingInfo::MacroName << "\n";
-OS << MarshallingInfo::MacroName << "(";
+OS << "#ifdef " << MI.getMacroName() << "\n";
+OS << MI.getMacroName() << "(";
 WriteOptRecordFields(OS, MI.R);
 OS << ", ";
 MI.emit(OS);
 OS << ")\n";
-OS << "#endif // " << MarshallingInfo::MacroName << "\n";
+OS << "#endif // " << MI.getMacroName() << "\n";
   }
 
   OS << "\n";
Index: llvm/include/llvm/Option/OptParser.td
===
--- llvm/include/llvm/Option/OptParser.td
+++ llvm/include/llvm/Option/OptParser.td
@@ -97,6 +97,7 @@
   OptionGroup Group = ?;
   Option Alias = ?;
   list AliasArgs = [];
+  code MacroPrefix = "";
   code KeyPath = ?;
   code DefaultValue = ?;
   code ImpliedValue = ?;
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -606,4 +606,19 @@
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-mad-enable")));
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-menable-unsafe-fp-math")));
 }
+
+// Diagnostic option.
+
+TEST_F(CommandLineTest, DiagnosticOptionPresent) {
+  const char *Args[] = {"-verify=xyz"};
+
+  ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
+
+  ASSERT_EQ(Invocation.getDiagnosticOpts().VerifyPrefixes,
+std::vector({"xyz"}));
+
+  Invocation.generateCC1CommandLine(GeneratedArgs, *this);
+
+  ASSERT_THAT(GeneratedArgs, ContainsN(StrEq("-verify=xyz"), 1));
+}
 } // anonymous namespace
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -386,7 +386,6 @@
 DiagnosticsEngine ,
 const InputArgList ) {
   LangOptions  = *Invocation.getLangOpts();
-  DiagnosticOptions  = Invocation.getDiagnosticOpts();
   CodeGenOptions  = Invocation.getCodeGenOpts();
   TargetOptions  = Invocation.getTargetOpts();
   FrontendOptions  = Invocation.getFrontendOpts();
@@ -400,8 +399,6 @@
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
-  llvm::sys::Process::UseANSIEscapeCodes(DiagOpts.UseANSIEscapeCodes);
-
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
@@ -1401,13 +1398,12 @@
 ARGS, DIAGS, SUCCESS, ID, FLAGS, PARAM, KEYPATH, DEFAULT_VALUE,\
 IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, MERGER, TABLE_INDEX) \
   if ((FLAGS)::CC1Option) {\
-this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);  \
+KEYPATH = MERGER(KEYPATH, DEFAULT_VALUE);  \
 if (IMPLIED_CHECK) \
-  this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);\
+  KEYPATH = MERGER(KEYPATH, IMPLIED_VALUE);\
 if (auto MaybeValue 

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 314887.
jansvoboda11 added a comment.

Rebase on top of prep patches


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -99,6 +99,13 @@
 
   MarshallingInfo(const Record ) : R(R) {}
 
+  std::string getMacroName() const {
+if (KeyPath.startswith("DiagnosticOpts."))
+  return (Twine("DIAG_") + MarshallingInfo::MacroName).str();
+
+return MarshallingInfo::MacroName;
+  }
+
   void emit(raw_ostream ) const {
 write_cstring(OS, StringRef(getOptionSpelling(R)));
 OS << ", ";
@@ -420,13 +427,13 @@
 MarshallingInfos.push_back(createMarshallingInfo(*R));
 
   for (const auto  : MarshallingInfos) {
-OS << "#ifdef " << MarshallingInfo::MacroName << "\n";
-OS << MarshallingInfo::MacroName << "(";
+OS << "#ifdef " << MI.getMacroName() << "\n";
+OS << MI.getMacroName() << "(";
 WriteOptRecordFields(OS, MI.R);
 OS << ", ";
 MI.emit(OS);
 OS << ")\n";
-OS << "#endif // " << MarshallingInfo::MacroName << "\n";
+OS << "#endif // " << MI.getMacroName() << "\n";
   }
 
   OS << "\n";
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -386,7 +386,6 @@
 DiagnosticsEngine ,
 const InputArgList ) {
   LangOptions  = *Invocation.getLangOpts();
-  DiagnosticOptions  = Invocation.getDiagnosticOpts();
   CodeGenOptions  = Invocation.getCodeGenOpts();
   TargetOptions  = Invocation.getTargetOpts();
   FrontendOptions  = Invocation.getFrontendOpts();
@@ -400,8 +399,6 @@
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
-  llvm::sys::Process::UseANSIEscapeCodes(DiagOpts.UseANSIEscapeCodes);
-
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
@@ -1401,13 +1398,12 @@
 ARGS, DIAGS, SUCCESS, ID, FLAGS, PARAM, KEYPATH, DEFAULT_VALUE,\
 IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, MERGER, TABLE_INDEX) \
   if ((FLAGS)::CC1Option) {\
-this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);  \
+KEYPATH = MERGER(KEYPATH, DEFAULT_VALUE);  \
 if (IMPLIED_CHECK) \
-  this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);\
+  KEYPATH = MERGER(KEYPATH, IMPLIED_VALUE);\
 if (auto MaybeValue =  \
 NORMALIZER(OPT_##ID, TABLE_INDEX, ARGS, DIAGS, SUCCESS))   \
-  this->KEYPATH = MERGER(  \
-  this->KEYPATH, static_castKEYPATH)>(*MaybeValue));   \
+  KEYPATH = MERGER(KEYPATH, static_cast(*MaybeValue));  \
   }
 
 bool CompilerInvocation::parseSimpleArgs(const ArgList ,
@@ -1428,86 +1424,34 @@
   return Success;
 }
 
-#undef PARSE_OPTION_WITH_MARSHALLING
-
 bool clang::ParseDiagnosticArgs(DiagnosticOptions , ArgList ,
 DiagnosticsEngine ,
 bool DefaultDiagColor) {
   bool Success = true;
 
-  Opts.DiagnosticLogFile =
-  std::string(Args.getLastArgValue(OPT_diagnostic_log_file));
+  DiagnosticOptions  = Opts;
+
+#define DIAG_OPTION_WITH_MARSHALLING(  \
+PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,\
+HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  \
+IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \
+TABLE_INDEX)   \
+  PARSE_OPTION_WITH_MARSHALLING(   \
+  Args, Diags, Success, ID, FLAGS, PARAM, KEYPATH, DEFAULT_VALUE,  \
+  IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, MERGER, TABLE_INDEX)
+#include "clang/Driver/Options.inc"
+#undef DIAG_OPTION_WITH_MARSHALLING
+
+  llvm::sys::Process::UseANSIEscapeCodes(Opts.UseANSIEscapeCodes);
+
   if (Arg *A =
   Args.getLastArg(OPT_diagnostic_serialized_file, OPT__serialize_diags))
 Opts.DiagnosticSerializationFile = A->getValue();
-  Opts.IgnoreWarnings = Args.hasArg(OPT_w);
-  Opts.NoRewriteMacros = 

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:102-107
+  std::string getMacroName() const {
+if (KeyPath.startswith("DiagnosticOpts."))
+  return (Twine("DIAG_") + MarshallingInfo::MacroName).str();
+
+return MarshallingInfo::MacroName;
+  }

jansvoboda11 wrote:
> dexonsmith wrote:
> > This seems like a bit of a semantic layering violation. It'd be pretty 
> > unexpected if someone renamed `DiagnosticOpts` in clang that they'd have to 
> > update this code in llvm. Is there another way to solve this problem?
> I don't like it either, but the alternatives I can think of are worse.
> 
> We could add a `string MacroPrefix;` field to LLVM's `Option` class and 
> populate it in Clang's TableGen file:
> 1. Via something like an `IsDiag` multiclass that we'd need to remember to 
> apply to each diagnostic option. I don't like it as it seems error prone and 
> introduces duplication.
> 2. Put all diagnostic options into a single `let MacroPrefix = "DIAG_" in { 
> ... }` block. This removes the duplication, but doesn't ensure an option is 
> in that block iff it's a diagnostic option with `"DiagnosticOpts.*"` keypath.
> 3. More involved approach would be to duplicate the LLVM's `Option` and 
> related stuff in Clang. That would get us a place to put the custom 
> `KeyPath.startswith("DiagnosticOpts.")` logic and then forward to LLVM's 
> `Option` with the appropriate `MacroPrefix`.
> 
> I'll think some more about it.
Doing #1 + #2 seems like an okay tradeoff to me (looking back at the old diff, 
it seems like that's similar to what @dang originally implemented (except 
adding a more generic `MacroPrefix`), and it seems fairly clean / obvious to 
me).

> [...] but doesn't ensure an option is in that block iff it's a diagnostic 
> option with "DiagnosticOpts.*" keypath.

The reason I'm okay with this is that I'm having trouble envisioning how this 
would go wrong practice.
- If someone adds somethings to `DiagnosticOptions`, they're likely to grep for 
how the adjacent field was defined / marshalled, duplicate the line, and modify 
it. I'm not seeing a likely path for them to copy/paste from a non-diagnostic 
option and/or miss adding this to the `let` block.
- If someone accidentally adds something to the `let` block that isn't in 
`DiagnosticOptions`, they'll get a compiler error in `ParseDiagnosticArgs`.

If you're still concerned, I wonder if there's a way to add a check in asserts 
builds that confirms that `ParseDiagnosticArgs` fills in `DiagnosticOptions` 
equivalently to how `createFromCommandLine` does? (and/or could the latter call 
the former as an implementation strategy?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:102-107
+  std::string getMacroName() const {
+if (KeyPath.startswith("DiagnosticOpts."))
+  return (Twine("DIAG_") + MarshallingInfo::MacroName).str();
+
+return MarshallingInfo::MacroName;
+  }

dexonsmith wrote:
> This seems like a bit of a semantic layering violation. It'd be pretty 
> unexpected if someone renamed `DiagnosticOpts` in clang that they'd have to 
> update this code in llvm. Is there another way to solve this problem?
I don't like it either, but the alternatives I can think of are worse.

We could add a `string MacroPrefix;` field to LLVM's `Option` class and 
populate it in Clang's TableGen file:
1. Via something like an `IsDiag` multiclass that we'd need to remember to 
apply to each diagnostic option. I don't like it as it seems error prone and 
introduces duplication.
2. Put all diagnostic options into a single `let MacroPrefix = "DIAG_" in { ... 
}` block. This removes the duplication, but doesn't ensure an option is in that 
block iff it's a diagnostic option with `"DiagnosticOpts.*"` keypath.
3. More involved approach would be to duplicate the LLVM's `Option` and related 
stuff in Clang. That would get us a place to put the custom 
`KeyPath.startswith("DiagnosticOpts.")` logic and then forward to LLVM's 
`Option` with the appropriate `MacroPrefix`.

I'll think some more about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:102-107
+  std::string getMacroName() const {
+if (KeyPath.startswith("DiagnosticOpts."))
+  return (Twine("DIAG_") + MarshallingInfo::MacroName).str();
+
+return MarshallingInfo::MacroName;
+  }

This seems like a bit of a semantic layering violation. It'd be pretty 
unexpected if someone renamed `DiagnosticOpts` in clang that they'd have to 
update this code in llvm. Is there another way to solve this problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 313347.
jansvoboda11 added a comment.

Remove unnecessary if braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -99,6 +99,13 @@
 
   MarshallingInfo(const Record ) : R(R) {}
 
+  std::string getMacroName() const {
+if (KeyPath.startswith("DiagnosticOpts."))
+  return (Twine("DIAG_") + MarshallingInfo::MacroName).str();
+
+return MarshallingInfo::MacroName;
+  }
+
   void emit(raw_ostream ) const {
 write_cstring(OS, StringRef(getOptionSpelling(R)));
 OS << ", ";
@@ -420,13 +427,13 @@
 MarshallingInfos.push_back(createMarshallingInfo(*R));
 
   for (const auto  : MarshallingInfos) {
-OS << "#ifdef " << MarshallingInfo::MacroName << "\n";
-OS << MarshallingInfo::MacroName << "(";
+OS << "#ifdef " << MI.getMacroName() << "\n";
+OS << MI.getMacroName() << "(";
 WriteOptRecordFields(OS, MI.R);
 OS << ", ";
 MI.emit(OS);
 OS << ")\n";
-OS << "#endif // " << MarshallingInfo::MacroName << "\n";
+OS << "#endif // " << MI.getMacroName() << "\n";
   }
 
   OS << "\n";
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -394,13 +394,12 @@
 IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \
 TABLE_INDEX)   \
   if ((FLAGS)::CC1Option) {\
-this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);  \
+KEYPATH = MERGER(KEYPATH, DEFAULT_VALUE);  \
 if (IMPLIED_CHECK) \
-  this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);\
+  KEYPATH = MERGER(KEYPATH, IMPLIED_VALUE);\
 if (auto MaybeValue =  \
 NORMALIZER(OPT_##ID, TABLE_INDEX, Args, Diags, Success))   \
-  this->KEYPATH = MERGER(  \
-  this->KEYPATH, static_castKEYPATH)>(*MaybeValue));   \
+  KEYPATH = MERGER(KEYPATH, static_cast(*MaybeValue));  \
   }
 
 /// Expects the following variables to be visible:
@@ -417,18 +416,17 @@
 [&](const auto ) {   \
   if (ALWAYS_EMIT ||   \
   (Extracted !=\
-   static_castKEYPATH)>(   \
-   (IMPLIED_CHECK) ? (IMPLIED_VALUE) : (DEFAULT_VALUE  \
+   static_cast((IMPLIED_CHECK) ? (IMPLIED_VALUE)\
+  : (DEFAULT_VALUE \
 DENORMALIZER(Args, SPELLING, SA, Option::KIND##Class, TABLE_INDEX, \
  Extracted);   \
-}(EXTRACTOR(this->KEYPATH));   \
+}(EXTRACTOR(KEYPATH)); \
   }
 
 static void FixupInvocation(CompilerInvocation ,
 DiagnosticsEngine ,
 const InputArgList ) {
   LangOptions  = *Invocation.getLangOpts();
-  DiagnosticOptions  = Invocation.getDiagnosticOpts();
   CodeGenOptions  = Invocation.getCodeGenOpts();
   TargetOptions  = Invocation.getTargetOpts();
   FrontendOptions  = Invocation.getFrontendOpts();
@@ -442,8 +440,6 @@
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
-  llvm::sys::Process::UseANSIEscapeCodes(DiagOpts.UseANSIEscapeCodes);
-
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
@@ -1447,82 +1443,22 @@
 bool DefaultDiagColor) {
   bool Success = true;
 
-  Opts.DiagnosticLogFile =
-  std::string(Args.getLastArgValue(OPT_diagnostic_log_file));
+  DiagnosticOptions  = Opts;
+
+#define DIAG_OPTION_WITH_MARSHALLING PARSE_OPTION_WITH_MARSHALLING
+#include "clang/Driver/Options.inc"
+#undef DIAG_OPTION_WITH_MARSHALLING
+
+  llvm::sys::Process::UseANSIEscapeCodes(Opts.UseANSIEscapeCodes);
+
   if (Arg *A =
   

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

I still need to figure out why Clang built in release mode replaces spaces in 
an error message by newlines and indents:

  $ ./build.noindex/debug/bin/clang -c clang/test/Driver/fmessage-length.c 
-fmessage-length=nan 
  clang-12: error: 
invalid
argument
'nan'
to
-fmessage-length=

Where the debug built Clang behaves as expected:

  $ ./build.noindex/debug/bin/clang -c clang/test/Driver/fmessage-length.c 
-fmessage-length=nan
  clang-12: error: invalid argument 'nan' to -fmessage-length=


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

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

[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 313337.
jansvoboda11 added a comment.

Rebase, undo NFC changes. Replace the previous diagnostic option tagging system 
implemented in TableGen with less intrusive backend check. Extract prep patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673

Files:
  clang/include/clang/Basic/DiagnosticOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -99,6 +99,14 @@
 
   MarshallingInfo(const Record ) : R(R) {}
 
+  std::string getMacroName() const {
+if (KeyPath.startswith("DiagnosticOpts.")) {
+  return (Twine("DIAG_") + MarshallingInfo::MacroName).str();
+}
+
+return MarshallingInfo::MacroName;
+  }
+
   void emit(raw_ostream ) const {
 write_cstring(OS, StringRef(getOptionSpelling(R)));
 OS << ", ";
@@ -420,13 +428,13 @@
 MarshallingInfos.push_back(createMarshallingInfo(*R));
 
   for (const auto  : MarshallingInfos) {
-OS << "#ifdef " << MarshallingInfo::MacroName << "\n";
-OS << MarshallingInfo::MacroName << "(";
+OS << "#ifdef " << MI.getMacroName() << "\n";
+OS << MI.getMacroName() << "(";
 WriteOptRecordFields(OS, MI.R);
 OS << ", ";
 MI.emit(OS);
 OS << ")\n";
-OS << "#endif // " << MarshallingInfo::MacroName << "\n";
+OS << "#endif // " << MI.getMacroName() << "\n";
   }
 
   OS << "\n";
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -394,13 +394,12 @@
 IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \
 TABLE_INDEX)   \
   if ((FLAGS)::CC1Option) {\
-this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);  \
+KEYPATH = MERGER(KEYPATH, DEFAULT_VALUE);  \
 if (IMPLIED_CHECK) \
-  this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);\
+  KEYPATH = MERGER(KEYPATH, IMPLIED_VALUE);\
 if (auto MaybeValue =  \
 NORMALIZER(OPT_##ID, TABLE_INDEX, Args, Diags, Success))   \
-  this->KEYPATH = MERGER(  \
-  this->KEYPATH, static_castKEYPATH)>(*MaybeValue));   \
+  KEYPATH = MERGER(KEYPATH, static_cast(*MaybeValue));  \
   }
 
 /// Expects the following variables to be visible:
@@ -417,18 +416,17 @@
 [&](const auto ) {   \
   if (ALWAYS_EMIT ||   \
   (Extracted !=\
-   static_castKEYPATH)>(   \
-   (IMPLIED_CHECK) ? (IMPLIED_VALUE) : (DEFAULT_VALUE  \
+   static_cast((IMPLIED_CHECK) ? (IMPLIED_VALUE)\
+  : (DEFAULT_VALUE \
 DENORMALIZER(Args, SPELLING, SA, Option::KIND##Class, TABLE_INDEX, \
  Extracted);   \
-}(EXTRACTOR(this->KEYPATH));   \
+}(EXTRACTOR(KEYPATH)); \
   }
 
 static void FixupInvocation(CompilerInvocation ,
 DiagnosticsEngine ,
 const InputArgList ) {
   LangOptions  = *Invocation.getLangOpts();
-  DiagnosticOptions  = Invocation.getDiagnosticOpts();
   CodeGenOptions  = Invocation.getCodeGenOpts();
   TargetOptions  = Invocation.getTargetOpts();
   FrontendOptions  = Invocation.getFrontendOpts();
@@ -442,8 +440,6 @@
   LangOpts.SpeculativeLoadHardening = CodeGenOpts.SpeculativeLoadHardening;
   LangOpts.CurrentModule = LangOpts.ModuleName;
 
-  llvm::sys::Process::UseANSIEscapeCodes(DiagOpts.UseANSIEscapeCodes);
-
   llvm::Triple T(TargetOpts.Triple);
   llvm::Triple::ArchType Arch = T.getArch();
 
@@ -1447,82 +1443,22 @@
 bool DefaultDiagColor) {
   bool Success = true;
 
-  Opts.DiagnosticLogFile =
-  std::string(Args.getLastArgValue(OPT_diagnostic_log_file));
+  DiagnosticOptions  = Opts;
+
+#define DIAG_OPTION_WITH_MARSHALLING PARSE_OPTION_WITH_MARSHALLING
+#include "clang/Driver/Options.inc"
+#undef DIAG_OPTION_WITH_MARSHALLING
+
+