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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
