[PATCH] clarify ownership of BugReport objects

[Aaron Ballman]

On Mon, Jun 22, 2015 at 1:59 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Jun 22, 2015 at 10:48 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> 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).
>
>
> Will leave it to Anna to sign off on, but looks like an improvement to me.
>
> (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?))

There are some derived classes. See RetainCountChecker.cpp
(CFRefLeakReport has an override).

~Aaron