[PATCH] D143751: [clang][analyzer][NFC] Refactor code of StdLibraryFunctionsChecker.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:83
+  }
+
   // The universal integral type to use in value range descriptions.
----------------
This enum is moved here from other location, and `negateKind` is added.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:124-125
   // requirement would render the initialization of the Summary map infeasible.
+  // A new value constraint is created furthermore by the negate functionality
+  // of the constraint and returned as pointer.
   using ValueConstraintPtr = std::shared_ptr<ValueConstraint>;
----------------
Szelethus wrote:
> Is this what you meant?
Probably this is better:
```
  // Mind that a pointer to a new value constraint can be created by negating an existing
  // constraint.
```
The important thing is that one reason for the shared pointer is the negate function that returns a pointer.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:218
+  /// Check if a single argument falls into a specific range.
+  /// The "range" can be built from sub-ranges that are closed intervals.
   class RangeConstraint : public ValueConstraint {
----------------
Szelethus wrote:
> If you want a killer doc, use examples. Not saying this is not good enough, but it wouldn't hurt.
The "range" is not the best term for this object in the documentation. A "range" is often used when a single interval is meant, but here it often means a set of ranges. I plan to improve this.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:271
+
+  private:
+    using RangeApplyFunction =
----------------
This new private section is the new added code.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:275
+
+    /// @brief Apply a function on all intervals in the range.
+    void applyOnWithinRange(
----------------
The "@brief" is probably not necessary and not used often, I plan to remove it.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:893
 
-std::string StdLibraryFunctionsChecker::NotNullConstraint::describe(
-    DescriptionKind DK, const CallEvent &Call, ProgramStateRef State,
-    const Summary &Summary) const {
-  SmallString<48> Result;
-  const auto Violation = ValueConstraint::DescriptionKind::Violation;
-  Result += "the ";
-  Result += getArgDesc(ArgN);
-  Result += " to '";
-  Result += getFunctionName(Call);
-  Result += DK == Violation ? "' should not be NULL" : "' is not NULL";
-  return Result.c_str();
+void StdLibraryFunctionsChecker::RangeConstraint::applyOnWithinRange(
+    BasicValueFactory &BVF, QualType ArgT, const RangeApplyFunction &F) const {
----------------
This code is got from `RangeConstraint::applyAsOutOfRange`.
This becomes "within" because in the apply as out of range case all ranges are cut out, that is really a loop over ("within") the ranges.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:910
+void StdLibraryFunctionsChecker::RangeConstraint::applyOnOutOfRange(
+    BasicValueFactory &BVF, QualType ArgT, const RangeApplyFunction &F) const {
+  if (Ranges.empty())
----------------
This code is got from `RangeConstraint::applyWithinRange`.
That was implemented by removal of all out-of-range intervals. The code here is a general version that calls a lambda instead of the actual assume calls, so it becomes reusable for other purposes when a loop over all (out-of) intervals is used.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1060
+                      const Summary &Summary,
+                      CheckerContext &C) const {
+  SValBuilder &SvalBuilder = C.getSValBuilder();
----------------
This function is just moved here from the previous (inline) location.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1143
+      if (ExplodedNode *N = C.generateErrorNode(State, NewNode))
+        reportBug(Call, N, Constraint.get(), NegatedConstraint.get(), Summary, C);
       break;
----------------
The new argument in `reportBug` is not used but is used in the next patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143751