On 01.03.2013 5:55, Jordan Rose wrote:
Memory allocated by malloc() should be deallocated by free(), not operator
delete[].
I'm really not sure whether it's better to mention the deallocator's expected
allocator, or the allocated region's expected deallocator. Which mistake do you
think people are more likely to make?
(Also, I came up with that phrasing in ten seconds; it could totally change.)
The second one of course. I definitely prefer to print the allocated region's
expected deallocator.
The single downside I see is that this breaks the uniformity of reports and
complicate ReportBadFree(). (other reports about, for example, freeing
addresses of labels, could not be printed in the new fashion)
Hm. I think we should still go with the second one.
One last major comment: in talking offline to Ted (Kremenek) about this, he pointed out
that it has the potential to be a very noisy new warning. In the same way that we have
unix.Malloc and alpha.unix.MallocWithAnnotations, it might be good to separate out
cplusplus.NewDelete and...we don't have a good category for MismatchedFree, but maybe
unix.MismatchedFree. That way we can control these checks individually; look for
"MallocOptimistic" to see how it's currently being used.
Have not got the idea yet. Please clarify what should
cplusplus.NewDelete and unix.MismatchedFree check for individually, and
what about other allocation families.
Can we just separate out the whole new check to something like
.MismatchedDeallocator?
Comments comments comments:
+// Used to check correspondence between allocators and deallocators.
+enum AllocationFamilies {
+ AF_None,
+ AF_Malloc,
+ AF_CXXNew,
+ AF_CXXNewArray
+};
Not sure if I introduced this or you, but we generally name enums in the
singular (AllocationFamily rather than AllocationFamilies).
- static RefState getReleased(const Stmt *s) { return RefState(Released, s); }
+ static RefState getReleased(const Stmt *s) {
+ return RefState(Released, s, AF_None);
+ }
static RefState getRelinquished(const Stmt *s) {
- return RefState(Relinquished, s);
+ return RefState(Relinquished, s, AF_None);
}
I'm wondering if you might as well preserve the families here. We're not using
them now, but at the very least it could be nice for debugging.
+ if (FD->getDeclName().getNameKind() != DeclarationName::CXXOperatorName)
+ return false;
+
+ OverloadedOperatorKind Kind = FD->getOverloadedOperator();
+ if (Kind != OO_New && Kind != OO_Array_New &&
+ Kind != OO_Delete && Kind != OO_Array_Delete)
+ return false;
The second check takes care of the first here.
+ OverloadedOperatorKind K = FD->getDeclName().getCXXOverloadedOperator();
Can be shortened to FD->getOverloadedOperator() again.
+ SmallString<100> TempBuf;
+ llvm::raw_svector_ostream TempOs(TempBuf);
+
+ if (printAllocDeallocName(TempOs, C, AllocExpr))
+ os << " was allocated by " << TempOs.str() << ", not ";
+ else
+ os << " was not allocated by ";
Ah, I see why you were avoiding this. Nevertheless, I think this is a better
experience for the user, and if we go with the new output this will get
restructured a bit anyway. (As a tiny comment, you can probably shrink the
SmallString to use less stack space, since most functions have much shorter
names.)
Jordan
--
Anton
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
