[PATCH] D73898: [analyzer] StdLibraryFunctionsChecker: Add argument constraints

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Just littering some more inlines, don't mind me :) Lets still wait on the dependency patch before updating.



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:296
+def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
+  HelpText<"Check constraints of arguments of C standard library functions">,
+  Dependencies<[StdCLibraryFunctionsChecker]>,
----------------
martong wrote:
> Szelethus wrote:
> > How about we add an example as well?
> You mean like NonNull or other constraints?
Like
```
Check constraints of arguments of C standard library functions, such as whether the parameter of isalpha is in the range [0, 255] or is EOF.
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:161
+
+    ValueRange negate() const {
+      ValueRange tmp(*this);
----------------
martong wrote:
> Szelethus wrote:
> > Maybe `complement` would be a better name? That sounds a lot more like a set operation. Also, this function highlights well that inheritance might not be the best solution here.
> Well, we check the argument constraint validity by trying to apply it's logical negation. In case of a range inclusion this is being out of that range. In case of non-null this is being null. And so on. The logic how we try to check an argument constraint is the same in all cases of the different constraints. And that is the point: in order to support a new kind of constraint we just have to figure out how to "apply" and "negate" one constraint. In my opinion this is a perfect case for polimorphism.
We agreed on inheritance in the previous patch, and regarding the name, sure, leave it as-is. :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:92-93
 
+  class ValueConstraint;
+  using ValueConstraintPtr = std::shared_ptr<ValueConstraint>;
   class ValueConstraint {
----------------
How about `ValueConstraintRef`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898