carlosgalvezp added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, + new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), ---------------- PiotrZSL wrote: > carlosgalvezp wrote: > > When downloading your patch, this seems to not be needed to make the tests > > pass, should it be removed? > No idea, it seem reasonable. Do you mean it seems reasonable to keep it, or reasonable to remove it? ================ Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83 Diags.setSourceManager(&Sources); + ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts()); ---------------- PiotrZSL wrote: > carlosgalvezp wrote: > > 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 ? > Yes, FIXME could be added, but ExpandModularHeadersPPCallbacks got also other > issues, for example with TargetTriple propagation and __has_builtin, looks > like all those got same source, simply we shoudn't create separate > Preprocessor. Agreed, the whole thing looks fishy. 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