aaron.ballman added inline comments.
================
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:
> > > > > > 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.
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
