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:
> > > > > 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.
> In that case, could you please revert the change to this line, since it's not
> needed?
Yes, but later today.
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
