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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL marked an inline comment as done.
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:
> PiotrZSL wrote:
> > carlosgalvezp wrote:
> > > 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?
> > reasonable to keep it, we want both DiagEngines to have same settings
> Reason I ask is that it seems the majority of `DiagnosticOptions` are initialized with default ctor:
> 
> ```
> $ git grep -E " DiagnosticOptions\(\w" | wc -l
> 3
> $ git grep -E " DiagnosticOptions\(\)" | wc -l
> 74
> ```
> 
> > we want both DiagEngines to have same settings
> 
> Do you know where "the other" `DiagEngine` is initialized? The one I can find is initialized without the compiler opts.
> 
> https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544
> 
> Since the added code does not have any impact on the result of the test, I'd argue that either the test is insufficient or the added code is dead code that should be removed.
sure, maybe ProcessWarningOptions will override it anyway, I didnt check more deeply.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156056/new/

https://reviews.llvm.org/D156056