Thank you! Commit in r240400. ~Aaron
On Tue, Jun 23, 2015 at 1:09 AM, Anna Zaks <[email protected]> wrote: > 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
