[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

Wed Sep 4 14:24:14 PDT 2019

aaron.ballman added a comment.

In D67140#1658353 <https://reviews.llvm.org/D67140#1658353>, @gribozavr wrote:

> In D67140#1658315 <https://reviews.llvm.org/D67140#1658315>, @aaron.ballman wrote:
>
> > In D67140#1656831 <https://reviews.llvm.org/D67140#1656831>, @NoQ wrote:
> >
> > > Honestly, i'm much more worried about message capitalization :)
> >
> >
> > Likewise. I wish the static analyzer would follow the usual conventions followed by clang and clang-tidy. ;-)
>
>
> I have the opposite opinion -- I wish that ClangTidy used complete sentences, and multiple sentences if it makes sense. The sentence fragments are too brief to explain complex and nuanced topics that ClangTidy communicates about. ClangTidy often plays the role of a developer education tool. It is not a guard like a compiler; developers can totally ignore ClangTidy if they disagree with the message. The better we can explain the problem, the more likely it is the developer will act on the message. I believe static analysis tools would be better off if we could write multiple sentences in the diagnostic.
>
> Even for compiler messages, a sentence fragment is sometimes too concise.

I agree with you in principle, but practicality still matters. I don't imagine we're going to go back and change the thousands of diagnostics in Clang to be complete sentences, and I prefer my diagnostics to be consistent. It's jarring when one part of the compiler uses one style of diagnostics and another part of the compiler uses a different style. So while I'd love it if we had more descriptive diagnostics, I would be happy to settle for consistent styles of diagnostics.

> 
> 
>> In D67140#1657421 <https://reviews.llvm.org/D67140#1657421>, @alexfh wrote:
>> 
>>> Historically, clang-tidy only used the term "check" (to denote the thing that checks something, rather than the rule being checked or the act of checking), and we tried to keep its use consistent. However, "checker" is a more precise and less ambiguous way to convey this meaning. I support to use the term "checker" in clang-tidy, as long as someone is willing to update the code and documentation (except for verbs, e.g. the `check()` method ;). Also note that there's a non-trivial number of out-of-tree check(er)s out there. They will need to be updated as well.
>>>
>>> Adding Aaron in case he has a different opinion.
>> 
>> 
>> My primary concern is with needless churn for out-of-tree clients. They don't get any real added benefit from the change in nomenclature, but renaming `ClangTidyCheck` to `ClangTidyChecker` will break every single out of tree clang-tidy checker. I've not used the plugin infrastructure for clang-tidy, but will this cause plugins to fail to load? If so, is it a silent failure or a noisy one?
> 
> A lot of the changes we do in Clang break many clients. I think we should try to find the best API.

Yes, but we also tend to push back on causing needless churn. Then again, with the recent resurfacing of discussions about renaming everything under the sun, maybe we've changed our community opinion here. :-D I guess I don't see Check vs Checker to be worthy of breaking everyone's out-of-tree code over.

> We could add a deprecated typedef for, say, one release, if you think that would be significantly helpful for out-of-tree users.

I think that's a pretty reasonable way forward. We should probably also put it in the release notes as well.

> The pre-compiled plugins will fail to load, but they will fail to load regardless of whether we make this change or not. The plugins will have to be recompiled anyway (and they will fail to compile), since Clang does not provide a stable library ABI.

Ah, good to know! That reduces my concern, but doesn't negate it. AFAIK, we haven't changed the interface such that it requires code changes rather than just a recompile in recent history, so this is a bit novel.

My personal feeling is: by itself, this isn't worth the churn but the fix to downstream code is pretty easy so I'm not strongly opposed. However, do we have other "I wish we changed this interface, but that would break the world" issues we want to tackle in this release for clang-tidy? That might make this refactoring more worth the pain.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67140