Page MenuHomePhabricator

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

Authored by sammccall on Nov 2 2018, 5:27 PM.

Details

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

Diff Detail

Event Timeline

sammccall created this revision.Nov 2 2018, 5:27 PM

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 accepted this revision.Nov 5 2018, 1:15 AM

The change looks reasonable to me.

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

This revision is now accepted and ready to land.Nov 5 2018, 1:15 AM

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?

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.

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.

This revision was automatically updated to reflect the committed changes.