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

[Justin Bogner via llvm-commits]

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.

>> 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.

With either of these changes buildSegments should probably assert that
ActiveRegions is empty though, for clarity.