carlosgalvezp accepted this revision.
carlosgalvezp added inline comments.
This revision is now accepted and ready to land.
================
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:
> > > > 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.
Ok! I will approve the patch now so you don't need to wait for me to land it
before branching tomorrow
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
