[PATCH] clarify ownership of BugReport objects

[Anna Zaks]

I have no additional comments. Looking forward to the updated patch.

Thank you,
Anna.
> On Jun 22, 2015, at 9:31 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 
> On Mon, Jun 22, 2015 at 12:23 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> I'm assuming emitReport always takes ownership? (it doesn't look like it has
>> any API surface area to express to the caller (in the current raw pointer
>> API) that it didn't take ownership)
> 
> Everywhere that I found was assuming ownership was taken, but I wanted
> to ask the analyzer folks in case they knew something I didn't.
> 
>> In that case, best to pass by value rather than (lvalue or rvalue) reference
>> so caller's explicitly pass off ownership (std::move(R)) to emitReport.
> 
> I was passing by reference on the chance that the caller did need the
> original pointer value, since it would be updated to something likely
> to crash quickly in the event they attempted to use the moved-from
> pointer value. My testing implies that no original caller uses the
> pointer after passing it to emitReport, so you're right, that probably
> should go back to being passed by value. Thank you!
> 
>> You'll have to use llvm::make_unique, rather than std::make_unique - since
>> we're still compiling as C++11, not C++14.
> 
> Oh shoot, that always messes me up! Easy enough to rectify.
> 
>> I probably wouldn't bother renaming "R" to "UniqueR" in emitReport - don't
>> think it adds/changes much. Ah, I see - there already was a UniqueR and R.
>> Probably just use R, though? (now that it's the only thing, so there's no
>> need to distinguish it as the unique one)
> 
> Yeah, I should go back to using R instead of UniqueR.
> 
>> Looks like a reformat of an unchanged/unrelated line:
>> 
>> -  BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
>> +  BugReportEquivClass *EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
> 
> Yeah, drive-by. I can rectify that separately.
> 
>> And where does emitReport actually take ownership?
> 
> Index: lib/StaticAnalyzer/Core/BugReporter.cpp
> ===================================================================
> --- lib/StaticAnalyzer/Core/BugReporter.cpp (revision 240274)
> +++ lib/StaticAnalyzer/Core/BugReporter.cpp (working copy)
> @@ -3224,11 +3224,8 @@
>   BugTypes = F.add(BugTypes, BT);
> }
> 
> -void BugReporter::emitReport(BugReport* R) {
> -  // To guarantee memory release.
> -  std::unique_ptr<BugReport> UniqueR(R);
> 
> That's where it was doing it.
> 
> Thank you for the review, David!
> 
> ~Aaron
> 
>> 
>> On Mon, Jun 22, 2015 at 8:07 AM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>> 
>>> Currently, when the analyzer wants to emit a report, it takes a naked
>>> pointer, and the emitReport function eventually takes over ownership
>>> of that pointer. I think this is a dangerous API because it's not
>>> particularly clear that this ownership transfer will happen, or that
>>> the pointer must be unique.
>>> 
>>> This patch makes the ownership semantics more clear by encoding it as
>>> part of the API. There should be no functional changes, and I do not
>>> think it caught any bugs, but I do think this is an improvement.
>>> 
>>> Thoughts or opinions?
>>> 
>>> ~Aaron
>>> 
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>> 
>>