dcoughlin added a comment. Thanks for iterating on the patch! Some comments in-line.
================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:569 + // allocating functions initialized to nullptr, which will never equal a + //non-null IdentifierInfo*, and never trigger on a non-Windows platform. + if (Ctx.getTargetInfo().getTriple().isWindowsMSVCEnvironment()) { ---------------- Please add a space here to align with the rest of the comment. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:622 + if (FD->getKind() == Decl::Function) { + if(isStandardNewDelete(FD, C)) + return false; ---------------- The identifier info is null for other operators as well (e.g., +). If you want to bail early when the identifier for a function is null, you should do so directly instead of trying to enumerate all cases. For example: ``` if (!FunI) return false; ``` This will be future-proof in case additional additional kinds of function declarations are added without identifiers. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:648 + } + } + ---------------- The indentation seems off here. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:664 + && FunI == II_win_alloca)) { + assert(!isStandardNewDelete(FD, C) && "We should not reach this point" + "with a C++ operator."); ---------------- You should remove this assert. Since you have guaranteed that you are targeting windows before checking and you initially II_win_alloca to a non-null value when targeting windows it can never be the case that II_win_alloca will be null here. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:814 + initIdentifierInfo(C.getASTContext()); + IdentifierInfo *FunI = FD->getIdentifier(); + ---------------- Do you want to do the same early bailout for an operator here also? ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:850 + && FunI == II_realloc_dbg) && + !isStandardNewDelete(FD, C.getASTContext())) { + State = ReallocMem(C, CE, false, State); ---------------- I don't think the `!isStandardNewDelete()` is needed here. Also, this is triggering -Wlogical-op-parentheses for me when building with clang. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:859 + FunI == II_calloc_dbg)) && + !isStandardNewDelete(FD, C.getASTContext())) { + State = CallocMem(C, CE, State); ---------------- Same here. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:866 + FunI == II_free_dbg)) && + !isStandardNewDelete(FD, C.getASTContext())) { + State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory); ---------------- Same here. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:874 + FunI == II_win_tempnam_dbg || FunI == II_wtempnam_dbg)) && + !isStandardNewDelete(FD, C.getASTContext())) { + State = MallocUpdateRefState(C, CE, State); ---------------- Same here. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:881 + FunI == II_win_alloca) && + !isStandardNewDelete(FD, C.getASTContext())) { + if (CE->getNumArgs() < 1) ---------------- Same here. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1183 + + //const FunctionDecl *FD = C.getCalleeDecl(CE); + //const IdentifierInfo *FI = FD->getIdentifier(); ---------------- You should remove the commented-out code. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1185 + //const IdentifierInfo *FI = FD->getIdentifier(); + assert(Att->getModule() != nullptr && "Only C++ operators should have a null" + "IdentifierInfo. We should not reach " ---------------- The explanation in the assert is not correct. This invariant holds because Sema guarantees that the module is not null when checking the attribute. See `handleOwnershipAttr()` for details. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1191 + (Att->getModule() != II_malloc_dbg) && + !isStandardNewDelete(C.getCalleeDecl(CE), C.getASTContext())) + return nullptr; ---------------- The '!isStandardNewDelete` check doesn't make any sense here. The code is dealing with the identifier in specified attribute and not the name of the called function. ================ Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1285 + + assert(Att->getModule() != nullptr && "Only C++ operators should have a null" + "IdentifierInfo. We should not reach " ---------------- Same here. ================ Comment at: llvm/tools/clang/test/Analysis/alternative-malloc-api.c:88 + +// What does "ContentIsDefined" refer to? +void testWinTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) { ---------------- I don't know the answer to this. Perhaps @zaks.anna recalls -- it looks like she wrote the test that this was copied from. But I don't think this comment adds much, do you? ================ Comment at: llvm/tools/clang/test/Analysis/alternative-malloc-api.c:95 + +// What does "ContentIsDefined" refer to? +void testWinWideTempnamNoLeakContentIsDefined(const wchar_t* dir, const wchar_t* prefix) { ---------------- Same here. https://reviews.llvm.org/D18073 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits