[clang-tidy] Add a diagnostic callback to parseConfiguration

[njames93]

Summary

Currently errors detected when parsing the YAML for .clang-tidy files are always printed to errs.
For clang-tidy binary workflows this usually isn't an issue, but using clang-tidy as a library for integrations may want to handle displaying those errors in their own specific way.

[njames93]

Use MemoryBufferRef instead of StringRef so the Diagnostic can have a meaningful filename.

[aaron.ballman]

Nothing is calling the new parseConfigurationWithDiags(), is that intentional? Also, is there a way to add test coverage for the change?

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

399

Would it make sense to require Handler to be nonnull with an assertion?

[njames93]

In 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.

Tests could be added

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

399

Wasn't sure which way to go with that one, happy to use an assert if you think it's a good idea

[aaron.ballman]

In D92920#2443748, @njames93 wrote:

In 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.

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

399

Now that I understand the use for this change better, I think the code is good as-is.

[njames93]

Added test cases.
Added assert to ensure Handler is valid.

[njames93]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

399

Whoops, just changed it to assert

[aaron.ballman]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

399

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.

[njames93]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

399

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.

[njames93]

Fix up test case now bug in yaml error locations has been fixed

[aaron.ballman]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

399

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.

[njames93]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

399

With a null handler, we can't capture any test output to validate anything.

[njames93]

Added back null check

[aaron.ballman]

LG!

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

399

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.

clang-tools-extra/clang-tidy/ClangTidyOptions.h

 /// Parses LineFilter from JSON and stores it to the \p Options.
 std::error_code parseLineFilter(llvm::StringRef LineFilter,
                                 ClangTidyGlobalOptions &Options);
 
 /// Parses configuration from JSON and returns \c ClangTidyOptions or an
 /// error.
 llvm::ErrorOr<ClangTidyOptions> parseConfiguration(llvm::StringRef Config);
 
+using DiagCallback = llvm::function_ref<void(const llvm::SMDiagnostic &)>;
+
+llvm::ErrorOr<ClangTidyOptions>
+parseConfigurationWithDiags(llvm::StringRef Config, DiagCallback Handler);
+
 /// Serializes configuration to a YAML-encoded string.
 std::string configurationAsText(const ClangTidyOptions &Options);
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYOPTIONS_H

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

llvm::ErrorOr<ClangTidyOptions> parseConfiguration(StringRef Config) {
   llvm::yaml::Input Input(Config);
   ClangTidyOptions Options;
   Input >> Options;
   if (Input.error())
     return Input.error();
   return Options;
 }
 
+static void diagHandlerImpl(const llvm::SMDiagnostic &Diag, void *Ctx) {
+  (*reinterpret_cast<DiagCallback *>(Ctx))(Diag);
+};
+
+llvm::ErrorOr<ClangTidyOptions>
+parseConfigurationWithDiags(StringRef Config, DiagCallback Handler) {
+  if (!Handler)
+    return parseConfiguration(Config);
+  llvm::yaml::Input Input(Config, nullptr, diagHandlerImpl, &Handler);
+  ClangTidyOptions Options;
+  Input >> Options;
+  if (Input.error())
+    return Input.error();
+  return Options;
+}
+
 std::string configurationAsText(const ClangTidyOptions &Options) {
   std::string Text;
   llvm::raw_string_ostream Stream(Text);
   llvm::yaml::Output Output(Stream);
   // We use the same mapping method for input and output, so we need a non-const
   // reference here.
   ClangTidyOptions NonConstValue = Options;
   Output << NonConstValue;
   return Stream.str();
 }
 
 } // namespace tidy
 } // namespace clang