LGTM and LGTJordan! Thank you, Anna.
> On Jun 22, 2015, at 11:03 AM, Aaron Ballman <[email protected]> wrote: > > On Mon, Jun 22, 2015 at 1:59 PM, David Blaikie <[email protected]> wrote: >> >> >> On Mon, Jun 22, 2015 at 10:48 AM, Aaron Ballman <[email protected]> >> wrote: >>> >>> Updated patch attached. >>> >>> On Mon, Jun 22, 2015 at 12:23 PM, David Blaikie <[email protected]> >>> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
