[PATCH] D96848: [clang][cli] Don't emit manufactured warnings

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 abandoned this revision.
jansvoboda11 added a comment.

Abandoning this in favor of D97041  & D97042 
.




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2347-2353
+// This warning was manufactured, don't put it on the command line.
+if (Warning == "no-stdlibcxx-not-found" && T.isOSDarwin() &&
+DashX.isPreprocessed())
+  continue;
+// This warning was manufactured, don't put it on the command line.
+if (Warning == "spir-compat" && T.isSPIR())
+  continue;

dexonsmith wrote:
> It seems reasonable to skip generating them if they're implied by other 
> command-line options, but I'm not sure "manufactured" is the right word to 
> use as a distinguishing characteristic. The entire CompilerInvocation could 
> have been created programmatically. I suggest instead saying the warning flag 
> is implied by the other command-line options.
> 
> Also, note that when created programmatically, one could have pushed 
> `stdlibcxx-not-found` *after* pushing `no-stdlibcxx-not-found`. Since that's 
> impossible to recreate, maybe there should be an assertion to catch this? 
> Alternatively, should this kind of imply-diagnostic-options logic be moved to 
> the driver?
> 
> Relatedly, unlike most command-line options, GenerateDiagnosticArgs is not 
> canonicalizing the options. For example, if `-Wabc` implies `-Wdef`, it'd be 
> nice to generate just `-Wabc` from initial command-lines of either `-Wabc 
> -Wdef` or `-Wdef -Wabc` / to drop the first of `-Wno-abc -Wabc` / etc.
> 
> IMO, something akin to an initial DiagnosticsEngine::DiagState (likely 
> renamed) could be stored in DiagnosticOptions (effectively, the resulting 
> state from calling ProcessWarningOptions). Parsing could translate 
> command-line options to this initial state. The state could be modified 
> programmatically; it'd also be used to initialize DiagnosticsEngine. 
> Generating command-line options would emit a canonical set of options that 
> would recreate the state. But that's a pretty big refactoring, and I think 
> it's okay to make progress without that.
> 
> As an initial fix, this is probably fine, but I think the comments and/or 
> FIXMEs should acknowledge that it's a bit fragile and point in a more sound / 
> less fragile direction (doesn't have to be my suggestion).
Using "implied" would be fitting as well. I guess I wanted to distinguish this 
from other implied options that don't need any explicit special-casing.

You're right the way of handling warnings is fragile. Normalizing the arguments 
into `DiagState` would be more robust, but as you say, it's a bit more 
involved. I'm putting that on my back-burner.

In the end, I looked into the history of `-Wno-stdlibcxx-not-found` and 
`-Wspir-compat` more closely and found ways to remove them from 
`CompilerInvocation` entirely: D97041 & D97042.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96848

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

[PATCH] D96848: [clang][cli] Don't emit manufactured warnings

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

dexonsmith added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2347-2353
+// This warning was manufactured, don't put it on the command line.
+if (Warning == "no-stdlibcxx-not-found" && T.isOSDarwin() &&
+DashX.isPreprocessed())
+  continue;
+// This warning was manufactured, don't put it on the command line.
+if (Warning == "spir-compat" && T.isSPIR())
+  continue;

It seems reasonable to skip generating them if they're implied by other 
command-line options, but I'm not sure "manufactured" is the right word to use 
as a distinguishing characteristic. The entire CompilerInvocation could have 
been created programmatically. I suggest instead saying the warning flag is 
implied by the other command-line options.

Also, note that when created programmatically, one could have pushed 
`stdlibcxx-not-found` *after* pushing `no-stdlibcxx-not-found`. Since that's 
impossible to recreate, maybe there should be an assertion to catch this? 
Alternatively, should this kind of imply-diagnostic-options logic be moved to 
the driver?

Relatedly, unlike most command-line options, GenerateDiagnosticArgs is not 
canonicalizing the options. For example, if `-Wabc` implies `-Wdef`, it'd be 
nice to generate just `-Wabc` from initial command-lines of either `-Wabc 
-Wdef` or `-Wdef -Wabc` / to drop the first of `-Wno-abc -Wabc` / etc.

IMO, something akin to an initial DiagnosticsEngine::DiagState (likely renamed) 
could be stored in DiagnosticOptions (effectively, the resulting state from 
calling ProcessWarningOptions). Parsing could translate command-line options to 
this initial state. The state could be modified programmatically; it'd also be 
used to initialize DiagnosticsEngine. Generating command-line options would 
emit a canonical set of options that would recreate the state. But that's a 
pretty big refactoring, and I think it's okay to make progress without that.

As an initial fix, this is probably fine, but I think the comments and/or 
FIXMEs should acknowledge that it's a bit fragile and point in a more sound / 
less fragile direction (doesn't have to be my suggestion).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96848

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

[PATCH] D96848: [clang][cli] Don't emit manufactured warnings

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The switch from per-member round-trip to whole-`CompilerInvocation` round-trip 
in D96280  means we'll be running the extra 
code present in `CompilerInvocation::CreateFromArgs`. This code conditionally 
manufactures extra warnings based on the target and input. This patch ensures 
we don't generate arguments for such manufactured warnings. Without this patch, 
each call to `CompilerInvocation::generateCC1CommandLine` during 
whole-`CompilerInvocation` round-trip could generate more arguments than the 
previous one.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96848

Files:
  clang/include/clang/Frontend/CompilerInvocation.h
  clang/lib/Frontend/CompilerInvocation.cpp


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2295,7 +2295,8 @@
 
 void CompilerInvocation::GenerateDiagnosticArgs(
 const DiagnosticOptions , SmallVectorImpl ,
-StringAllocator SA, bool DefaultDiagColor) {
+StringAllocator SA, bool DefaultDiagColor, const llvm::Triple ,
+InputKind DashX) {
   const DiagnosticOptions *DiagnosticOpts = 
 #define DIAG_OPTION_WITH_MARSHALLING(  
\
 PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
@@ -2343,6 +2344,13 @@
 // This option is automatically generated from UndefPrefixes.
 if (Warning == "undef-prefix")
   continue;
+// This warning was manufactured, don't put it on the command line.
+if (Warning == "no-stdlibcxx-not-found" && T.isOSDarwin() &&
+DashX.isPreprocessed())
+  continue;
+// This warning was manufactured, don't put it on the command line.
+if (Warning == "spir-compat" && T.isSPIR())
+  continue;
 Args.push_back(SA(StringRef("-W") + Warning));
   }
 
@@ -4831,11 +4839,12 @@
 void CompilerInvocation::generateCC1CommandLine(
 SmallVectorImpl , StringAllocator SA) const {
   llvm::Triple T(TargetOpts->Triple);
+  InputKind DashX = FrontendOpts.DashX;
 
   GenerateFileSystemArgs(FileSystemOpts, Args, SA);
   GenerateMigratorArgs(MigratorOpts, Args, SA);
   GenerateAnalyzerArgs(*AnalyzerOpts, Args, SA);
-  GenerateDiagnosticArgs(*DiagnosticOpts, Args, SA, false);
+  GenerateDiagnosticArgs(*DiagnosticOpts, Args, SA, false, T, DashX);
   GenerateFrontendArgs(FrontendOpts, Args, SA, LangOpts->IsHeaderFile);
   GenerateTargetArgs(*TargetOpts, Args, SA);
   GenerateHeaderSearchArgs(*HeaderSearchOpts, Args, SA);
Index: clang/include/clang/Frontend/CompilerInvocation.h
===
--- clang/include/clang/Frontend/CompilerInvocation.h
+++ clang/include/clang/Frontend/CompilerInvocation.h
@@ -249,7 +249,8 @@
   /// Generate command line options from DiagnosticOptions.
   static void GenerateDiagnosticArgs(const DiagnosticOptions ,
  SmallVectorImpl ,
- StringAllocator SA, bool 
DefaultDiagColor);
+ StringAllocator SA, bool DefaultDiagColor,
+ const llvm::Triple , InputKind DashX);
 
   /// Parse command line options that map to LangOptions.
   static bool ParseLangArgsImpl(LangOptions , llvm::opt::ArgList ,


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2295,7 +2295,8 @@
 
 void CompilerInvocation::GenerateDiagnosticArgs(
 const DiagnosticOptions , SmallVectorImpl ,
-StringAllocator SA, bool DefaultDiagColor) {
+StringAllocator SA, bool DefaultDiagColor, const llvm::Triple ,
+InputKind DashX) {
   const DiagnosticOptions *DiagnosticOpts = 
 #define DIAG_OPTION_WITH_MARSHALLING(  \
 PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,\
@@ -2343,6 +2344,13 @@
 // This option is automatically generated from UndefPrefixes.
 if (Warning == "undef-prefix")
   continue;
+// This warning was manufactured, don't put it on the command line.
+if (Warning == "no-stdlibcxx-not-found" && T.isOSDarwin() &&
+DashX.isPreprocessed())
+  continue;
+// This warning was manufactured, don't put it on the command line.
+if (Warning == "spir-compat" && T.isSPIR())
+  continue;
 Args.push_back(SA(StringRef("-W") + Warning));
   }
 
@@ -4831,11 +4839,12 @@
 void CompilerInvocation::generateCC1CommandLine(
 SmallVectorImpl , StringAllocator SA) const {
   llvm::Triple