This is an archive of the discontinued LLVM Phabricator instance.

Add getSetRef() to SetVector
AbandonedPublic

Authored by hintonda on Mar 6 2016, 9:25 PM.

Details

Summary

Access to the underlying set implementation lets users call
methods like find, if the set implementation supports them.

See D17884 (DIImportedEntitys) for an example of when this might be
important -- need to keep a unique set, but also need the order to be
deterministic.

Switching from DenseSet to SetVector, even if the underlying set
implementation is DenseSet, isn't possible since the caller can't
check to see if the key has already been added without access to the
underlying set.

Diff Detail

Event Timeline

hintonda updated this revision to Diff 49931.Mar 6 2016, 9:25 PM
hintonda retitled this revision from to Add getSetRef() to SetVector.
hintonda updated this object.
hintonda added reviewers: rafael, dblaikie, aaboud.
hintonda added a subscriber: llvm-commits.
aaboud edited edge metadata.Mar 7 2016, 1:56 AM

I do not think we need this new method, no need to access the underlying set directly.

  1. If you use "insert()" you assure that the element will be added only once to the SetVector.
  2. If you want to check if the element is already in the SetVector, you may use the function "count()".

I agree that it isn't absolutely necessay, however, it is much more efficient. Relying on insert means you have create the object you want to insert first. In the case of DIImportEntitys, that means new'ing a new one.

We get around that by looking up the key first, which is possible for DenseSet, but not possible with SetVector, at least not unless you provide an escape hatch and return a reference to the underlying.

hintonda abandoned this revision.Apr 17 2016, 9:33 PM