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

Nathan James via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Dec 10 13:20:35 PST 2020


njames93 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);
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92920



More information about the cfe-commits mailing list