[PATCH] clarify ownership of BugReport objects

[David Blaikie]

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


>
> ~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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150622/af6b3386/attachment.html>