Thanks Anton!
I have no more comments. Jordan does not have time to review, so you are all
set.
Looking forward to the follow up patches!
Anna.
================
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:368-370
@@ -367,3 +367,5 @@
/// parameters to the given call.
- /// \param IsConst Specifies if the pointer is const.
+ /// \param Kind The reason of pointer escape.
+ /// \param HTraits Information about special handling for a particular
+ /// region/symbol.
/// \returns Checkers can modify the state by returning a new one.
----------------
Антон Ярцев wrote:
> Anna Zaks wrote:
> > I know this is not new to this patch, but PointerEscapeKind and the traits
> > info are very related...
> >
> > Would it make sense to extend the kind enum and use it for traits? One
> > benefit I see is that all the info about the escape would be in one place.
> > We would only have one checker callback.. On the other hand, I don't recall
> > why they are different concepts now..
> I don't particulary like the idea to keep traits into the PointerEscapeKind
> enum.
> They are related, but at the same time they differ in their meaning and
> nature.
> Escape kinds describe reasons of a pointer escapes, kinds are mutually
> exclusive.
> Traits currently describe invalidation traits (preserve contents, invalidate
> indirect regions, etc.). Any number of traits may be attached to a region at
> the
> same time.
> Escape kinds and traits are independent - each escape kind may come with any
> set
> of traits.
>
> Foud the reason for the separate ConstEscape callback in out mail
> correspondence:
> Re: [PATCH][Review request] unix.Malloc checker improvement: +handling
> new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved
> display names for allocators/deallocators
> 29.03.2013 4:00
> >>>>>>>> Anton, I think handling this similarly to pointer escape is the best.
> >>>>>>>> However, I am concerned that if we simply extend pointer escape to
> >>>>>>>> trigger on another "Kind" of escape, all the other users of
> >>>>>>>> pointerEscape will need to know about it (and do nothing for this
> >>>>>>>> kind
> >>>>>>>> of escape). How about a new checker callback, which will rely on the
> >>>>>>>> same core code as _checkPointerEscape? This way the checker
> >>>>>>>> developers
> >>>>>>>> would not need to know about this special case, and we can reuse all
> >>>>>>>> the code that determines when the escape should be triggered.
> >>>>>> Anna, As for me it would be better to leave the single callback as
> >>>>>> this
> >>>>>> is just another type of pointer escape, if I understand correctly.
> >>>>>> Have no other arguments for this :)
> >>>>>> In addition, the "Kind" approach is relatively new, so hopefully a few
> >>>>>> users of pointerEscape be affected.
> >>>> Anton, The main concern is that every user of the callback will now have
> >>>> to check if the kind is ConstPointer (or whatever we call it, maybe
> >>>> multiple kinds..) and do nothing in that case. So every user will need
> >>>> to
> >>>> know about this special case and handle it specially.
> >> Anton, I've committed the new callback. Looks like no email was sent out..
> >> So just look at r178310.
>
OK.
================
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:377
@@ -374,3 +376,3 @@
PointerEscapeKind Kind,
- bool IsConst = false);
+ RegionAndSymHandlingTraits *HTraits = NULL);
----------------
Антон Ярцев wrote:
> Anna Zaks wrote:
> > When does it make sense for traits to be missing? (Why the NULL
> > initialization? Also, I think it should be '0', not "NULL")
> Currently pointer escape on bind do not deal with traits ( ExprEngine.cpp,
> processPointerEscapedOnBind() ), NULL initialization just allows to leave the
> code of processPointerEscapedOnBind() unchanged. This is the only reason for
> NULL initialization. Do you think NULL initialization should be removed?
>
> Changed "NULL" to "0". Searched LLVM code standard for "0" vs "NULL", found
> nothing. What is wrong with "NULL"? :)
>
Is that expected to change? Maybe we should just pass NULL at that one call
site, instead of using default initialization.
There are probably threads discussing which one to use "NULL" or "0" and I
assume we came to a decision to use '0'. I can see arguments on both sides, one
is more expressive, the other one is shorter and cannot be redefined...
http://llvm-reviews.chandlerc.com/D1486
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
