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

Reply via email to