<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 4, 2016 at 12:07 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Vedant Kumar <<a href="mailto:vsk@apple.com">vsk@apple.com</a>> writes:<br>
<span class="">> David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> writes:<br>
>> This seems like more than a performance fix - the API itself seems a little<br>
>> problematic. If buildSegments was called more than once on the same<br>
>> SegmentBuilder, tehre would be trouble, right? (before this patch, it could<br>
>> build on top of existing segments - after this patch it will use a<br>
>> moved-from-vector which might be in some non-empty state (moved from<br>
>> objects are in a "valid but unspecified" state - I'm not sure there's<br>
>> anything other than empty that a vector could be in, but it's still a bit<br>
>> subtle/risky))<br>
<br>
</span>Calling buildSegments more than once was probably already a bug - it<br>
would certainly do something a bit strange, anyway.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
>> Could the API be redesigned so it doesn't have this problem?<br>
><br>
</span><span class="">> I agree with dblaikie, the API could be a bit cleaner. Wdyt of this:<br>
><br>
> 1. Have the SegmentBuilder constructor accept a<br>
> `vector<CoverageSegment>&`; assert that it's empty.<br>
> 2. Replace `Segments = SegmentBuilder().buildSegments(...)` with<br>
> `SegmentBuilder(Segments).build(...)`.<br>
<br>
</span>We can keep the API the same but make Segments a local in buildSegments<br>
and pass it by reference to popRegion and startSegment. Then NRVO will<br>
remove the copy but we keep a simple API.<br></blockquote><div><br></div><div>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?)<br><br>That way you won't need the std::move either - the local will implicitly move out on return.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">With either of these changes buildSegments should probably assert that<br>
ActiveRegions is empty though, for clarity.<br>
</blockquote></div><br></div></div>