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