[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders

[Carlos Galvez via Phabricator via cfe-commits]

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