[PATCH] clarify ownership of BugReport objects

Aaron Ballman aaron at aaronballman.com
Mon Jun 22 10:48:08 PDT 2015


Updated patch attached.

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

Fixed.

>
> You'll have to use llvm::make_unique, rather than std::make_unique - since
> we're still compiling as C++11, not C++14.

Fixed.

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

Fixed.

> Looks like a reformat of an unchanged/unrelated line:
>
> -  BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
> +  BugReportEquivClass *EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);

Removed (will commit separately).

~Aaron

>
> And where does emitReport actually take ownership?
>
> 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
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unique.patch
Type: application/octet-stream
Size: 62522 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150622/5f33ecfa/attachment.obj>


More information about the cfe-commits mailing list