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

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

Thanks for adding the macros. I've provided some feedback inline.

I think a good rule of thumb for readability is: suppose you are a maintainer and need to add a summary for a new function. Can you copy the the summary for an existing function and figure out what each component means so you can change it for the new function?


================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:419
@@ -418,1 +418,3 @@
 
+def StdLibraryFunctionsChecker : Checker<"StdLibraryFunctions">,
+  HelpText<"Improve modeling of standard library functions">,
----------------
I know you and Gábor already discussed this -- but shouldn't this be CStdLibraryFunctionsChecker or 'StdCLibraryFunctionsChecker'? Or is is your intent that both C and C++ standard libraries would be modeled by this checker?

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:11
@@ +10,3 @@
+// This checker improves modeling of a few simple library functions.
+// It does not throw warnings.
+//
----------------
"throw" --> "generate"

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:534
@@ +533,3 @@
+    // The isascii() family of functions.
+    SPEC {
+      FOR_FUNCTION(isalnum),
----------------
Is "specification" the right term here? Or is this really a "summary"?

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:536
@@ +535,3 @@
+      FOR_FUNCTION(isalnum),
+      SPEC_DATA {
+        ARGUMENT_TYPES { IntTy },
----------------
'SPEC_DATA' doesn't seem to add much in terms of readability. Is it needed?

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:537
@@ +536,3 @@
+      SPEC_DATA {
+        ARGUMENT_TYPES { IntTy },
+        RETURN_TYPE(IntTy),
----------------
The argument and return types seem like more a property of the function than than the summary. Why are they here and not with the function name?

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:540
@@ +539,3 @@
+        INVALIDATION_APPROACH(EvalCallAsPure),
+        BRANCHES {
+          BRANCH { // Boils down to isupper() or islower() or isdigit()
----------------
"Cases" seems more appropriate than "branches" (branching is an implementation detail).

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:542
@@ +541,3 @@
+          BRANCH { // Boils down to isupper() or islower() or isdigit()
+            RANGE {
+              ARG_NO(0), RANGE_KIND(WithinRange),
----------------
If I understand correctly, the first "argument" to branch describes the constraints on the function arguments and the second (if present) describes the resulting constraint on the return value when the argument constraint holds. Is there a way to make this apparent in the spelling of the summary?

As a straw proposal, what about renaming the first 'RANGE' to 'ARGUMENT_CONSTRAINT' and the second the 'RETURN_VALUE_CONSTRAINT'?

Or, more jargony, "PRECONDITION" and "POSTCONDITION"?

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:547
@@ +546,3 @@
+            RANGE {
+              RET_VAL, RANGE_KIND(OutOfRange),
+              SET { POINT(0) }
----------------
Is it ever the case that this final 'RANGE" constrains anything other than the return value? If not, can 'RET_VAL' be elided?

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:554
@@ +553,3 @@
+              ARG_NO(0), RANGE_KIND(WithinRange),
+              SET { SEG(128, 255) }
+            }
----------------
What is the motivation behind the use of geometric terms here (i.e., "SEG", "POINT")? Why not "INTERVAL" and "EXACT_VALUE"?

================
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:560
@@ +559,3 @@
+              ARG_NO(0), RANGE_KIND(OutOfRange),
+              SET { SEG('0', '9') U SEG('A', 'Z')
+                                  U SEG('a', 'z') U SEG(128, 255)}
----------------
Would you be opposed to 'UNION' instead of 'U'?


https://reviews.llvm.org/D20811