[PATCH] ADT: Add SmallPtrSet::reset()

[David Blaikie]

On Thu, Mar 19, 2015 at 5:50 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015-Mar-19, at 16:52, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > Totally plausible. Optional/debatable comments:
> >
> > 1) is it worth supporting the more idiomatic clear+shrink_to_fit to be
> clear of the semantics? ('reset' seems vague) It looks like it's sort of
> halfway there already, with shrink_and_clear. Could just be generalized a
> bit more to be part of the solution to this use case
>
> `reset()` is used with smart pointers to mean "like it's default
> constructed", so that's where I was going with it.  I realize this
> isn't related to pointers though :).
>
> I hadn't considered `shrink_to_fit()`, and at first blush I liked it
> better than what I chose.
>
> However, unlike `std::vector<>` where `clear()` is "free",
> `SmallPtrSet<>::clear()` is going to `malloc()` a new array.  I guess
> another option is to change `clear()` to shrink down to small storage
> (a bigger change, but it saves a malloc).  Thoughts on that approach?
>

It seems like totally the right thing - clear() causing allocation sounds
pretty bad to me. Lazily wait until there's a use to cause allocation.

Though the malloc is partly because clear() is semi-smart, trying to shrink
a bit (but not too much? Why? perhaps because it expects it'll grow to a
similar size, but not quite as big? This seems too smart for its own good,
really)


>
> If I decide to stick with the current model, do you have any better
> names than `reset()`?
>

shrink_to_empty() ? But yeah, clear seems better - and I'd wonder if we
should actually break the old clear behavior (maybe check revision history
to see if it was introduced for any particular calles that should become
clear+shrink_to_fit) - it's not unreasonable for a caller to expect clear()
not to deallocate the existing buffer so it can grow again to that size
with little/no overhead.


>
> > 2) I would perhaps consider testing via looking at the address of an
> element added after calling reset, rather than using isSmall, but I realize
> that's totally subjective (& reactive - perhaps if I wrote it, I would've
> written it as you have)
>
> Right, right.  I was being lazy here...
>
> >
> > On Thu, Mar 19, 2015 at 4:43 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > 0001 adds `SmallPtrSet::reset()`, which returns `SmallPtrSet` to small
> > mode after completely freeing its heap memory.  To facilitate testing,
> > I changed `isSmall()` to a public accessor.
> >
> > Thoughts?
> >
> > (0002 is the reason I was looking at this.)
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150319/ef1634c9/attachment.html>