PiotrZSL added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75
+ Diags(new DiagnosticIDs,
+ new DiagnosticOptions(Compiler.getDiagnosticOpts()),
new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),
----------------
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.
================
Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83
Diags.setSourceManager(&Sources);
+ ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
----------------
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.
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
