[PATCH] D69813: [analyzer] CERTStrChecker: Model gets()

Wed Nov 6 11:34:01 PST 2019

aaron.ballman added a comment.

In D69813#1735804 <https://reviews.llvm.org/D69813#1735804>, @Szelethus wrote:

> In D69813#1734272 <https://reviews.llvm.org/D69813#1734272>, @Charusso wrote:
>
> > In D69813#1734193 <https://reviews.llvm.org/D69813#1734193>, @Szelethus wrote:
> >
> > > Hmm, so this checker is rather a collection of CERT rule checkers, right? Shouldn't the checker name contain the actual rule name (STR31-C)? User interfacewise, I would much
> >
>
>
> Implementationwise that sounds wonderful, these rules are fairly similar to have a single checker responsible for them. The user interface however (enabling different CERT rules) they should probably be split up into separate checkers per rule, rather than options, wouldn't you agree? @NoQ?

I'm not @NoQ, but I do agree that there should be a separate check per rule in terms of the UI presented to the user. The name should follow the rule ID like they do in clang-tidy, for some consistency there.

> Also,  `cert.str.Termination` sounds like a wonderful name, I don't insist on the rule number being present in it, but at the very least, it should be in the description.

I think that the rule number should be in the name. I'd probably go with `cert.STR31-C` or `cert.str31-c` (so it's clear which CERT standard the rule came from).

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

https://reviews.llvm.org/D69813