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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
