[PATCH] D20811: [analyzer] Model some library functions

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Thanks for the patch! Here are the comments, most of which are nits.

Could you add the high level description of what we are doing somewhere or maybe just describe the meaning of FunctionSpec since it encodes how functions are modeled.

Also, we should explain why we are not using BodyFarm somewhere in the comment.


================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:10
@@ +9,3 @@
+//
+// This checker improves modeling of a few simple library functions.
+// It does not throw warnings.
----------------
Please, list the functions.

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:27
@@ +26,3 @@
+  static const uint32_t Ret = std::numeric_limits<uint32_t>::max();
+  enum ValueRangeKindTy { Outside, Inside, ComparesToArgument };
+  enum InvalidationKindTy { Normal, Pure };
----------------
Naming looks odd: maybe "OutOfRange" and "WithinRange"?

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:33
@@ +32,3 @@
+  private:
+    // ArgNo in CallExpr and CallEvent is defined is Unsigned, but
+    // obviously uint32_t should be enough for most practical purposes.
----------------
nit: "is Unsigned" -> "as Unsigned"
Please, use a typedef for the type as you are using it below in getArgType.

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:83
@@ +82,3 @@
+  static QualType getArgType(const FunctionSpec &Spec, uint32_t ArgNo) {
+    return ArgNo == Ret ? Spec.RetType : Spec.ArgTypes[ArgNo];
+  }
----------------
Are the types in FunctionSpec already canonical? If so, please, add a comment.

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:178
@@ +177,3 @@
+        if (auto N = V.getAs<NonLoc>()) {
+          const IntRangeVector &R = VR.getRanges();
+          size_t E = R.size();
----------------
nit: If you could factor these out into separate helper functions, it would be easier to read. Lot's of nesting..

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:219
@@ +218,3 @@
+
+    if (NewState)
+      C.addTransition(NewState);
----------------
What happens when NewState == State? I guess addTransition would just not do anything, but maybe we should just make the intent explicit and not call it at all.

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:245
@@ +244,3 @@
+  }
+  case Normal:
+    return false;
----------------
Replace "Normal" with a more descriptive name.

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:273
@@ +272,3 @@
+
+  // Verify that function signature matches the spec in advance,
+  // so that we didn't have to roll back if anything goes wrong.
----------------
Looks like you might want to have the checking code as a member on FunctionSpec.

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:322
@@ +321,3 @@
+
+  // NOTE: The signature needs to have the correct number of arguments.
+  // NOTE: However, insert Irrelevant when the type is insignificant.
----------------
Let's explain what we are doing next. For example, "Let's initialize the FunctionSpec for the functions we are modeling."

Remove "NOTE:"

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:326
@@ +325,3 @@
+  //       is completely unknown, omit it from the respective range set.
+  // NOTE: Upon comparing to another argument, the other argument is casted
+  //       to the current argument's type. This avoids proper promotion but
----------------
not clear where this is used or if it is used in the initialization at all.

================
Comment at: lib/StaticAnalyzer/Checkers/LibraryFunctionsChecker.cpp:337
@@ +336,3 @@
+  //      { ranges list:
+  //        { argument index, inside or outside, {{from, to}, ...} },
+  //        { argument index, compares to argument, {{how, which}} },
----------------
Is every item in the range set used to bifurcate the state?


http://reviews.llvm.org/D20811