Page MenuHomePhabricator

[analyzer] Provide Contains() on ImmutableMap program state partial trait.
ClosedPublic

Authored by ddcc on Nov 7 2016, 3:09 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

ddcc updated this revision to Diff 77109.Nov 7 2016, 3:09 PM
ddcc retitled this revision from to [analyzer] Provide Contains() on ImmutableMap program state partial trait..
ddcc updated this object.
ddcc added reviewers: zaks.anna, dcoughlin.
ddcc added a subscriber: cfe-commits.
dcoughlin accepted this revision.Nov 7 2016, 4:38 PM
dcoughlin edited edge metadata.

This seems reasonable and it looks good to me (as long as there is a later patch coming that calls the new method).

However: is there a situation where Contains() is preferable to Lookup()? It seems to me that in some cases you would want the looked-up value -- and in those where you don't there is no cost to return a pointer to it. As far as I can tell both ultimately end up as a call to find() on the AVL tree.

This revision is now accepted and ready to land.Nov 7 2016, 4:38 PM
ddcc added a comment.Nov 8 2016, 11:43 AM

Even though there isn't a performance difference, I think it is semantically clearer since it is explicit that the value is unneeded.

The interface of ProgramState provides a contains() function that calls into Contains() of the underlying partial traits as part of its implementation. That function is present for ImmutableSet and ImmutableList, so it is inconsistent that ImmutableMap doesn't have it. I've been working on a Z3 constraint backend that uses this, though the implementation has been trickier than I expected.

In D26373#589614, @ddcc wrote:

Even though there isn't a performance difference, I think it is semantically clearer since it is explicit that the value is unneeded.

Makes sense to me!

This revision was automatically updated to reflect the committed changes.