[PATCH] D92920: [clang-tidy] Add a diagnostic callback to parseConfiguration

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LG!



================
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:
> > njames93 wrote:
> > > aaron.ballman wrote:
> > > > njames93 wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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.
> > > > > Whoops, just changed it to assert
> > > > Heh, sorry about that! Before changing it again, let's make sure we agree. My thinking is: this is a general API (rather than a specific one only to be used internally) and not every caller may care about reporting diagnostics; the error return code is sufficient to tell any caller whether the parsing was successful or not.
> > > That's pretty much it, For external users. If they don't care about capturing the diagnostics then the `parseConfiguration` function should be used. If they do care and call this new function with an empty callable, they have probably made a mistake. 
> > Okay, then I apologize for the churn, but can you go back to accepting a null input (and a test case for it)? With that, I think the patch LG.
> With a null handler, we can't capture any test output to validate anything.
I meant more just to demonstrate that it's a supported part of the interface and won't crash, assert, or do bad things. If you don't think it will be that useful, I don't insist.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92920/new/

https://reviews.llvm.org/D92920