<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 22, 2015 at 10:48 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Updated patch attached.<br>
<span class=""><br>
On Mon, Jun 22, 2015 at 12:23 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
</span><span class="">> I'm assuming emitReport always takes ownership? (it doesn't look like it has<br>
> any API surface area to express to the caller (in the current raw pointer<br>
> API) that it didn't take ownership)<br>
><br>
> In that case, best to pass by value rather than (lvalue or rvalue) reference<br>
> so caller's explicitly pass off ownership (std::move(R)) to emitReport.<br>
<br>
</span>Fixed.<br>
<span class=""><br>
><br>
> You'll have to use llvm::make_unique, rather than std::make_unique - since<br>
> we're still compiling as C++11, not C++14.<br>
<br>
</span>Fixed.<br>
<span class=""><br>
> I probably wouldn't bother renaming "R" to "UniqueR" in emitReport - don't<br>
> think it adds/changes much. Ah, I see - there already was a UniqueR and R.<br>
> Probably just use R, though? (now that it's the only thing, so there's no<br>
> need to distinguish it as the unique one)<br>
<br>
</span>Fixed.<br>
<span class=""><br>
> Looks like a reformat of an unchanged/unrelated line:<br>
><br>
> -  BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);<br>
> +  BugReportEquivClass *EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);<br>
<br>
</span>Removed (will commit separately).<br></blockquote><div><br>Will leave it to Anna to sign off on, but looks like an improvement to me.<br><br>(my usual generic question on changes like this (should probably be treated separately from this patch, if you've interest): could we use this by value instead of by pointer? I see BugReport has virtual functions but not sure it has any derived classes (the Doxygen docs don't seem to indicate any?))<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> And where does emitReport actually take ownership?<br>
><br>
> On Mon, Jun 22, 2015 at 8:07 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> Currently, when the analyzer wants to emit a report, it takes a naked<br>
>> pointer, and the emitReport function eventually takes over ownership<br>
>> of that pointer. I think this is a dangerous API because it's not<br>
>> particularly clear that this ownership transfer will happen, or that<br>
>> the pointer must be unique.<br>
>><br>
>> This patch makes the ownership semantics more clear by encoding it as<br>
>> part of the API. There should be no functional changes, and I do not<br>
>> think it caught any bugs, but I do think this is an improvement.<br>
>><br>
>> Thoughts or opinions?<br>
>><br>
>> ~Aaron<br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>><br>
><br>
</div></div></blockquote></div><br></div></div>