[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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156056