[PATCH] clarify ownership of BugReport objects

[Aaron Ballman]

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