<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jul 6, 2017 at 11:04 AM David Li via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">davidxl added a comment.<br>
<br>
Limit NamedInstrProfRecord to the reader file seems ok -- but this class also looks like a useful convenience class. We can probably discuss that in a different thread if it is desirable.<br></blockquote><div><br>Sure<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<br>
================<br>
Comment at: lib/ProfileData/InstrProfWriter.cpp:180<br>
+Error InstrProfWriter::addRecord(NamedInstrProfRecord &&I, uint64_t Weight) {<br>
+  // Careful/subtle code: the std::move(I) below doesn't adversely affect the<br>
+  // Name and Hash fields, since the third parameter to addRecord takes<br>
----------------<br>
I don't find the comment here helping.  Perhaps just mention the move construction happens later when assigned to the Dest?<br></blockquote><div><br>I refactored the code to hopefully make it obviously correct without the confusing comment:<br><br>  auto Name = I.Name;<br>  auto Hash = I.Hash;<br>  func(Name, Hash, std::move(I), ...)<br><br>There are about 3 different reasons this code didn't have a potentially used-after-move bug in it. (one was that it passes by rvalue ref, rather than by value - so the std::move has no effect at this call site, the other is that only the base subobject is moved, and the third is that StringRef and uint64_t are trivially movable (and such a trivial move is actually a copy, so it leaves the original objects untouched) - but none of that's really obvious from the code one is looking at right here, so perhaps writing the defensive/obviously correct code that doesn't rely on any of those things is better)<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D34838" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34838</a><br>
<br>
<br>
<br>
</blockquote></div></div>