I have no additional comments. Looking forward to the updated patch. Thank you, Anna. > On Jun 22, 2015, at 9:31 AM, Aaron Ballman <[email protected]> wrote: > > 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) > > Everywhere that I found was assuming ownership was taken, but I wanted > to ask the analyzer folks in case they knew something I didn't. > >> 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. > > I was passing by reference on the chance that the caller did need the > original pointer value, since it would be updated to something likely > to crash quickly in the event they attempted to use the moved-from > pointer value. My testing implies that no original caller uses the > pointer after passing it to emitReport, so you're right, that probably > should go back to being passed by value. Thank you! > >> You'll have to use llvm::make_unique, rather than std::make_unique - since >> we're still compiling as C++11, not C++14. > > Oh shoot, that always messes me up! Easy enough to rectify. > >> 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) > > Yeah, I should go back to using R instead of UniqueR. > >> Looks like a reformat of an unchanged/unrelated line: >> >> - BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos); >> + BugReportEquivClass *EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos); > > Yeah, drive-by. I can rectify that separately. > >> And where does emitReport actually take ownership? > > Index: lib/StaticAnalyzer/Core/BugReporter.cpp > =================================================================== > --- lib/StaticAnalyzer/Core/BugReporter.cpp (revision 240274) > +++ lib/StaticAnalyzer/Core/BugReporter.cpp (working copy) > @@ -3224,11 +3224,8 @@ > BugTypes = F.add(BugTypes, BT); > } > > -void BugReporter::emitReport(BugReport* R) { > - // To guarantee memory release. > - std::unique_ptr<BugReport> UniqueR(R); > > That's where it was doing it. > > Thank you for the review, David! > > ~Aaron > >> >> On Mon, Jun 22, 2015 at 8:07 AM, Aaron Ballman <[email protected]> >> 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 >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
