[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 <https://reviews.llvm.org/D97041> & D97042 <https://reviews.llvm.org/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