Szelethus added a comment. 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 prefer smaller, leaner checkers than a big one > > with a lot of options, which are barely supported for end-users. I would > > expect a `cert` package to contain subpackages like `cert.str`, and checker > > names `cert.str.31StringSize`, or similar. > > > It is the STR rules of CERT, nothing else. Most of the rules are tied > together, and that is why the checker needs to be designed as one checker at > first. I am not sure which part of the STR I will cover, so may when the > checker evolves and some functions does not need the same helper methods we > need to create new checkers. STR31 and STR32 are my projects which is like > one single project. Also I did not except the users to specify the rule > number, but this checker could be something like `cert.str.Termination`. > There is two floating-point CERT checkers inside the `insecureAPI` that is > why I have introduced the `cert` package, which will have three members, one > is that new checker. I think a new package is only necessary if it contains > at least two checkers. 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? 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. >> Also, shouldn't we move related checkers from `security.insecureAPI` to >> `cert`? Or just mention the rule name in the description, and continue not >> having a `cert` package? > > We should not, they does not fit into CERT rules, but it has two CERT > floating-point checkers. The `cert` package should be well described with > CERT rules. I want to move the CERT checkers from it into that `cert` > package, and leave the rest. I meant //related// ones only, though I didn't go through the checkers individually, it might just be the case that `insecureApi` doesn't implement any specific CERT rules :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69813/new/ https://reviews.llvm.org/D69813 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
