aaron.ballman added a comment. In D92920#2443748 <https://reviews.llvm.org/D92920#2443748>, @njames93 wrote:
> In D92920#2443578 <https://reviews.llvm.org/D92920#2443578>, @aaron.ballman > wrote: > >> Nothing is calling the new `parseConfigurationWithDiags()`, is that >> intentional? Also, is there a way to add test coverage for the change? > > Yes it is intentional, the idea is for programs embedding clang-tidy to use > this interface if they wish to handle diagnostics from parsing themselves. Ahh, I see, thank you for the explanation. > Tests could be added I think they should be added. ================ Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:400 + DiagCallback Handler) { + llvm::yaml::Input Input(Config, nullptr, Handler ? diagHandlerImpl : nullptr, + &Handler); ---------------- njames93 wrote: > aaron.ballman wrote: > > Would it make sense to require `Handler` to be nonnull with an assertion? > Wasn't sure which way to go with that one, happy to use an assert if you > think it's a good idea Now that I understand the use for this change better, I think the code is good as-is. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92920/new/ https://reviews.llvm.org/D92920 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
