[PATCH] D14014: Checker of proper vfork usage

[Anna Zaks via cfe-commits]

zaks.anna added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:45
@@ +44,3 @@
+                                   CheckerContext &C) {
+  const Expr *CE = Call.getOriginExpr();
+
----------------
ygribov wrote:
> It seems that other checkers do more or less the same throw-away predicates (e.g. see isAssignmentOp in DereferenceChecker.cpp).
> Frankly this function only handles two common patterns right now 

> It seems that other checkers do more or less the same throw-away 
> predicates

Both of these just highlight that it's best to make this a utility function: 
 - New checkers will not have to roll their own predicates.
 - The reusable code will get enhanced in the future and all checkers will benefit from it.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:64
@@ +63,3 @@
+
+  // Check for variable initialization.
+  auto PD = dyn_cast_or_null<DeclStmt>(P);
----------------
Ah, right!

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:209
@@ +208,3 @@
+      && !isCallWhitelisted(Call.getCalleeIdentifier(), C))
+    reportBug("Calling functions (except exec*() or _exit())", C);
+}
----------------
Using regex in a diagnostic is not user friendly. See the comment below.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:227
@@ +226,3 @@
+
+  reportBug("Assigning variables (except return value of vfork)", C);
+}
----------------
It is very important to keep the diagnostics as succinct as possible.

The diagnostics in this checker are long as is and I do not think the "except" clauses are helping the user to understand or fix the problem. They will not get the warning in the cases that are described in the "except" clause so why are we talking about this..

Most likely you are using it to make sure that the message is true. How about rephrasing the message to address that issue. For example, you could use something like:
"Assigning variables (except return value of vfork) is prohibited after a successful fork"
->
"This assignment is prohibited after a successful vfork"


http://reviews.llvm.org/D14014