>>> 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.
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
