[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