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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:418
+    if (FailureSt && !SuccessSt) {
+      C.addTransition(FailureSt);
+      if (ExplodedNode *N = C.generateErrorNode(FailureSt))
----------------
balazske wrote:
> Is this `addTransition` needed? It would be OK to call `generateErrorNode` with `State`. Even if not, adding the transition before should not be needed?
Yes, you are right it is superfluous, I removed it.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:688
                   // locale-specific return values.
                   .Case({ArgumentCondition(0U, WithinRange, {{128, UCharMax}})})
                   .Case({ArgumentCondition(0U, OutOfRange,
----------------
balazske wrote:
> Why is this `{128, UCharMax}` here and at the next entry needed?
This is the local specific range , [128, 255]. There are characters like `ä` which we don't know if they are treated as an alphanumerical character or not. We can't really tell how a specific libc implementation classifies them. On the other hand, with English letters we can state the classes confidently.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:696
+                  .ArgConstraint(ArgumentCondition(
+                      0U, WithinRange, {{EOFv, EOFv}, {0, UCharMax}}))},
       },
----------------
balazske wrote:
> Is this `ArgConstraint` intentionally added only to `isalnum`?
> 
Yes, I wanted to create first the infrastructure and then later to add all these constraints to the rest of the summaries with new tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898