[PATCH] D18756: [Coverage] Avoid unnecessary copying of an array. NFC.

[David Blaikie via llvm-commits]

On Mon, Apr 4, 2016 at 12:07 PM, Justin Bogner <mail at justinbogner.com>
wrote:

> Vedant Kumar <vsk at apple.com> writes:
> > David Blaikie <dblaikie at gmail.com> writes:
> >> This seems like more than a performance fix - the API itself seems a
> little
> >> problematic. If buildSegments was called more than once on the same
> >> SegmentBuilder, tehre would be trouble, right? (before this patch, it
> could
> >> build on top of existing segments - after this patch it will use a
> >> moved-from-vector which might be in some non-empty state (moved from
> >> objects are in a "valid but unspecified" state - I'm not sure there's
> >> anything other than empty that a vector could be in, but it's still a
> bit
> >> subtle/risky))
>
> Calling buildSegments more than once was probably already a bug - it
> would certainly do something a bit strange, anyway.
>

Right - but that's my point, the API allows it but it would be buggy to do
so. Seems like a weird/flawed API to have that feature.


>
> >> Could the API be redesigned so it doesn't have this problem?
> >
> > I agree with dblaikie, the API could be a bit cleaner. Wdyt of this:
> >
> > 1. Have the SegmentBuilder constructor accept a
> > `vector<CoverageSegment>&`; assert that it's empty.
> > 2. Replace `Segments = SegmentBuilder().buildSegments(...)` with
> > `SegmentBuilder(Segments).build(...)`.
>
> We can keep the API the same but make Segments a local in buildSegments
> and pass it by reference to popRegion and startSegment. Then NRVO will
> remove the copy but we keep a simple API.
>

Yep, that'd be cool with me/seems nice/reasonable, so far as I can tell
from a very naive glance at the code (is there other state in
SegmentBuilder that would need to be reset on subsequent calls to
buildSegments?)

That way you won't need the std::move either - the local will implicitly
move out on return.


> With either of these changes buildSegments should probably assert that
> ActiveRegions is empty though, for clarity.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160404/7e51af6f/attachment.html>