[PATCH] D14014: Checker of proper vfork usage

[Aleksei Sidorin via cfe-commits]

a.sidorin added a comment.

I put some suggestions in inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:47
@@ +46,3 @@
+
+  bool isChildProcess(const ProgramStateRef State) const;
+
----------------
I think it's a good idea to make some functions static and/or move them out of class definition.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:97
@@ +96,3 @@
+
+    ASTContext &Ctx = C.getASTContext();
+    for (const char **id = ids; *id; ++id)
----------------
What about combination of std::transform + std::sort + std::binary_search?

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:136
@@ +135,3 @@
+    const DeclStmt *PD = dyn_cast_or_null<DeclStmt>(P);
+    if (!PD)
+      break;
----------------
```
if (PD && PD->isSingleDecl()) {
  if (const VarDecl *D = dyn_cast<VarDecl>(PD->getSingleDecl())
   return D;
```

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:149
@@ +148,3 @@
+
+  // see if it's an ordinary assignment
+  do {
----------------
You can use early return to escape do{}.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:196
@@ +195,3 @@
+  SVal VforkRetVal = Call.getReturnValue();
+  SymbolRef Sym = VforkRetVal.getAsSymbol();
+  Optional<DefinedOrUnknownSVal> DVal =
----------------
Is check for non-concrete value is really required?

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:252
@@ +251,3 @@
+  ProgramStateRef State = C.getState();
+  if (!isChildProcess(State))
+    return;
----------------
```
if (is...)
  report...
```


Repository:
  rL LLVM

http://reviews.llvm.org/D14014