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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
