On Apr 5, 2013, at 2:05 PM, Anton Yartsev <[email protected]> wrote:
> On 06.04.2013 0:30, Jordan Rose wrote: >> On Apr 5, 2013, at 13:22 , Anton Yartsev <[email protected]> wrote: >> >>> On 05.04.2013 21:55, Jordan Rose wrote: >>>> if (!Filter.CMallocOptimistic && !Filter.CMallocPessimistic && >>>> - !Filter.CNewDeleteChecker) >>>> + !Filter.CNewDeleteLeaksChecker) >>>> return; >>>> - if (!isTrackedFamily(C, Sym)) >>>> + const RefState *RS = C.getState()->get<RegionState>(Sym); >>>> + assert(RS && "cannot leak an untracked symbol"); >>>> + AllocationFamily Family = RS->getAllocationFamily(); >>>> + if (!isTrackedFamily(Family)) >>>> return; >>>> + // Special case for new and new[]; these are controlled by a separate >>>> checker >>>> + // flag so that they can be selectively disabled. >>>> + if (Family == AF_CXXNew || Family == AF_CXXNewArray) >>>> + if (!Filter.CNewDeleteLeaksChecker) >>>> + return; >>> We already checked for Filter.CNewDeleteLeaksChecker at the beginning. >>> >>> Now this works properly just because isTrackedFamily() does not know about >>> CNewDeleteLeaksChecker and returns false. >>> What about adding bug types at this point? >> The first check says "any of >> {MallocOptimistic,MallocPessimistic,NewDeleteLeaks}". This one checks >> exclusively for NewDeleteLeaks and AF_CXXNew[Array]. > Oops, sorry. > >> This is basically a quick-and-dirty "bug types" implementation that's used >> because today we only have one bug type that's different. I'm not sure the >> added generalization will actually make things clearer—here we implicitly >> know the "type" is a leak because we're in reportLeak. None of the rest of >> the code has to know that leaks are any different from anything else. But >> maybe I'm missing some benefit of the general bug types. > No benefits other then just looks clearer to me :) Passing a bug kind shows, > on the one hand, that sometimes we depend on the bug type, and allows to > freely add other bug-dependent code on the other. > I quickly returned the bug types while waited for your replay, just to see, > if it simplify the appearance, attached the patch so you could also see how > it looks like. > I am under no circumstances insist on adding bug types, sending just for > information, perhaps you like it. :) > Pulling all the decisions about when to warn based on the family into a single function seems to be an improvement. However, introducing an enum of 6 bug types, while only one of the values is used is odd and probably redundant. > In any case, these may be slightly simplified as isTrackedFamily() now treats > AF_None as unacceptable (after r178899 ) >> I'll admit it's a bit kludgy that the family is effectively checked twice, >> but I actually like the implication that alpha.cplusplus.NewDeleteLeaks is a >> "sub-checker" of cplusplus.NewDelete. >> >> Jordan > > -- > Anton > > <MallocChecker.cpp.BugTypes.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
