carlosgalvezp added a comment. A thought came to mind - since we are doing workarounds anyway, would it be easier to ask people to simply add `-clang-diagnostic*` to the `Checks` in their config file? It's fair to assume they will get those warnings when compiling the code. I feel the more workarounds we add in the code the harder it will be to clean it up later :)
================ Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, + new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), ---------------- When downloading your patch, this seems to not be needed to make the tests pass, should it be removed? ================ Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83 Diags.setSourceManager(&Sources); + ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts()); ---------------- PiotrZSL wrote: > carlosgalvezp wrote: > > A bit unclear to me why we should add this line here, grepping for this > > function in the repo I only find hits in the `clang` folder. How come it's > > not needed in other places? > We create here new Preprocessor (line 96) and new DiagEngine (line 74), when > C++20/Modules are enabled this class is register as an second Preprocessor > and both are (+-) executed. > Unfortunately when we pass `-Wno-macro-redefined` it's pass only to original > DiagEngine, and we run into situation when warning is suppressed by first > DiagEngine, but not by second that is used by second Preprocessor. > > Passing DiagnosticOptions alone to DiagEngine looks to be insufficient, as > it's does not apply settings, only calling this function apply them. > (somehow). > This is gray area for me. > > More about problem here: > https://discourse.llvm.org/t/rfc-expand-modular-headers-ppcallbacks-problem-in-c-20/71628 Thanks for the explanation! I'm not sure what the best way forward is. Would it make sense to add some `TODO` or `FIXME` comment to further investigate in the future if we want that line of code ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits