[PATCH] clarify ownership of BugReport objects

[Anna Zaks]

LGTM and LGTJordan!

Thank you,
Anna.

> On Jun 22, 2015, at 11:03 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 
> 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).
Correct.
> 
> ~Aaron