Charusso added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2549-2552 + FunctionStr = Lexer::getSourceText( + CharSourceRange::getTokenRange( + {FD->getBeginLoc(), FD->getBody()->getBeginLoc()}), + C.getSourceManager(), C.getLangOpts()); ---------------- NoQ wrote: > I'm slightly worried that it'll crash when `free()` is being called from > within a body farm. > > For now it probably cannot happen because none of the bodyfarmed functions > can call `free()` directly, but i'd anyway rather add a check that the source > locations we're taking are valid. Oh, I missed that, thanks! I wanted to check for everything, yes. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2554-2559 + if (FunctionStr.equals("")) + return false; + + // We do not model the Integer Set Library's retain-count based allocation. + if (!FunctionStr.contains("__isl_")) + return false; ---------------- NoQ wrote: > If the string is empty, it clearly cannot contain `__isl_`, so the first > check is redundant. The first check is: "We got a body and its decl?", the second check is: "We got an ISL macro?". Yea, it is kind of redundant, just I like to pack one check in one IfStmt, and now that two question merges. Also I like to make them one-liners so they are self-explanatory. Here is an example why I like it: ``` // Escape pointers passed into the list, unless it's an ObjC boxed // expression which is not a boxable C structure. if (!(isa<ObjCBoxedExpr>(Ex) && !cast<ObjCBoxedExpr>(Ex)->getSubExpr() ->getType()->isRecordType())) ``` - from `ExprEngine::Visit()` - `Expr::ObjCBoxedExprClass` case. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2569 + if (const RefState *RS = State->get<RegionState>(Sym)) { + State = State->remove<RegionState>(Sym); + State = State->set<RegionState>(Sym, RefState::getEscaped(RS)); ---------------- NoQ wrote: > `remove` is unnecessary, we overwrite it anyway. I believe in so as well, just the official code base has this semantic. I have rewritten that, see below in `checkPointerEscapeAux()`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64680/new/ https://reviews.llvm.org/D64680 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits