[clang-tidy] run() doesn't update the SourceManager.

[sammccall]

Summary

By now the context's SourceManager is now initialized everywhere that
ClangTidyCheck::registerMatcher() is called, so the call from run() seems
entirely redundant, and indeed all the tests pass.

This solves a problem with embedding clang-tidy: if using a DiagnosticsEngine
which already has file state, re-setting its SourceManager (to the same value)
causes an assertion.
(There are other ways to solve this problem, but this is the simplest).

[steveire]

After this change, it seems that ClangTidyCheck::check is not needed and all callers should be ported to call run() instead (and the private run() declaration should be removed from ClangTidyCheck.

[hokein]

The change looks reasonable to me.

In D54061#1286505, @steveire wrote:

After this change, it seems that ClangTidyCheck::check is not needed and all callers should be ported to call run() instead (and the private run() declaration should be removed from ClangTidyCheck.

Theoretically, we could replace ClangTidyCheck::check with ClangTidyCheck::run, but I'm not sure it is worth, ClangTidyCheck::check is a public API, and is widely-used (for all clang-tidy checks), replacing it requires large changes (although it is one-line change), it might break downstream clang-tidy checks.

clang-tidy/ClangTidy.cpp

444

I'd add an assertion assert(Context->hasSourceManager()), but it turns out ClangTidyContext doesn't provide this method...

[JonasToth]

Theoretically, we could replace ClangTidyCheck::check with ClangTidyCheck::run, but I'm not sure it is worth, ClangTidyCheck::check is a public API, and is widely-used (for all clang-tidy checks), replacing it requires large changes (although it is one-line change), it might break downstream clang-tidy checks.

We can add a deprecation warning and remove the check method in the
next version?

[sammccall]

In D54061#1286956, @JonasToth wrote:

Theoretically, we could replace ClangTidyCheck::check with ClangTidyCheck::run, but I'm not sure it is worth, ClangTidyCheck::check is a public API, and is widely-used (for all clang-tidy checks), replacing it requires large changes (although it is one-line change), it might break downstream clang-tidy checks.

We can add a deprecation warning and remove the check method in the
next version?

As Haojian says, this is a lot of churn.
I don't think the benefit of pushing people to migrate, even with a grace period, is clear.

Additionally it's *possible* that future refactorings may mean we want to run additional "framework" logic when the MatchFinder calls us, and so requiring ClangTidy::check and MatchCallback::run to be the same function seems a little risky.

I'll add a comment explaining why the two functions exist.

[JonasToth]

In D54061#1288395, @sammccall wrote:

In D54061#1286956, @JonasToth wrote:

Theoretically, we could replace ClangTidyCheck::check with ClangTidyCheck::run, but I'm not sure it is worth, ClangTidyCheck::check is a public API, and is widely-used (for all clang-tidy checks), replacing it requires large changes (although it is one-line change), it might break downstream clang-tidy checks.

We can add a deprecation warning and remove the check method in the
next version?

As Haojian says, this is a lot of churn.
I don't think the benefit of pushing people to migrate, even with a grace period, is clear.

Additionally it's *possible* that future refactorings may mean we want to run additional "framework" logic when the MatchFinder calls us, and so requiring ClangTidy::check and MatchCallback::run to be the same function seems a little risky.

I'll add a comment explaining why the two functions exist.

Alright.

clang-tidy/ClangTidy.cpp

 }
 
 DiagnosticBuilder ClangTidyCheck::diag(SourceLocation Loc, StringRef Message,
                                        DiagnosticIDs::Level Level) {
   return Context->diag(CheckName, Loc, Message, Level);
 }
 
 void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) {
-  Context->setSourceManager(Result.SourceManager);
+  // For historical reasons, checks don't implement the MatchFinder run()
+  // callback directly. We keep the run()/check() distinction to avoid interface
+  // churn, and to allow us to add cross-cutting logic in the future.
   check(Result);
 }
 
 OptionsView::OptionsView(StringRef CheckName,
                          const ClangTidyOptions::OptionMap &CheckOptions)
     : NamePrefix(CheckName.str() + "."), CheckOptions(CheckOptions) {}
 
 std::string OptionsView::get(StringRef LocalName, StringRef Default) const {