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

Thu Sep 5 14:08:24 PDT 2019

aaron.ballman added a comment.

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

> In D67140#1659356 <https://reviews.llvm.org/D67140#1659356>, @aaron.ballman wrote:
>
> > In D67140#1658969 <https://reviews.llvm.org/D67140#1658969>, @gribozavr wrote:
> >
> > > In D67140#1658365 <https://reviews.llvm.org/D67140#1658365>, @aaron.ballman wrote:
> > >
> > > > 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.
> > >
> > >
> > > I think API changes happen all the time. At Google, we are integrating upstream LLVM and Clang changes into our internal codebase daily. We have a lot of internal ClangTidy checkers. Fixing up all our internal code to keep with upstream changes is a full time job for one engineer (but it is a rotation).
> >
> >
> > I think this sort of backs up the point I was making.
>
>
> Sorry, but in the previous comment you were saying that "we haven't changed the interface such that it requires code changes". So which is it, for you?

Both. :-) It's been my experience that there are relatively few breaking changes to clang-tidy checks (but they certainly do happen). Your experience is that you need an entire FTE to deal with the amount of breaks. Both of our experiences can be valid simultaneously.

> 
> 
>> It requires an FTE to keep up with the breaks already.
> 
> Exactly. So one more or one less API change that requires a trivial code change *does not matter*. People working on the Clang upgrade still have a ton of other, complex, changes to deal with. Is it more work to upgrade for this sort of API rename? Yes. Is meaningfully more work, if you look at everything that needs to be done as a whole? No. It will not be a thing that makes or breaks someone's Clang upgrade.

That's one perspective on it, yes. As I said, my experience is different than yours, which is probably why I'm more hesitant to make sweeping breaking changes like this when they have so little tangible benefit. You claim it won't make or break someone's Clang upgrade, but it certainly *can*. I've worked for places that, when we didn't have the resources to deal with breakage from a compiler upgrade, we delayed upgrading to that compiler until after we handled the issues raised, and I don't think this was all that unusual of a practice.

I'm not saying *no* to this change, despite my push-back.  We don't have source compatibility or API stability guarantees, so it would be unreasonable to think we'll never make a breaking change. I just don't think it's a good software engineering practice to be cavalier about breaking 100% of downstream users without a very good reason to do so. My opinion is that check vs checker is not a very good reason to break the world, but I'm not willing to hold up the patch over it if that opinion isn't shared by the community.

> Why do static analysis tools have to be consistent with Clang in its message style?

I don't think it's a requirement (so long as the diagnostics are clear about the issue being diagnosed, I'm happy enough), but I think it's good for a tool to be self-consistent in its messaging. It's jarring that one part of the compiler emits diagnostics one way and another emits them a totally different way. It may be less of an impact for people who don't see the output from both at the same time, but that happens to be the use case I have. As a somewhat related example of inconsistencies, the Clang static analyzer does not report compiler diagnostics through it's bug reporter interface, which means that exporting analysis results to SARIF only exports the static analyzer bugs. This has caught quite a few people by surprise at GrammaTech and they've asked when Clang can start reporting all of the diagnostics instead of just some of them (aka, they thought this behavior was a bug), but then the inconsistencies in messaging would be more obvious.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67140