This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] NFC: Replace intrusive list of bug reports with a vector of pointers.
ClosedPublic

Authored by NoQ on Aug 30 2019, 3:28 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Aug 30 2019, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2019, 3:28 PM
gribozavr accepted this revision.Sep 2 2019, 2:04 AM

Thanks for the simplification!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
460 ↗(On Diff #218177)

I don't think we intend users to use ReportList, so it would be better to not expose it. Instead:

using iterator = SmallVectorImpl<std::unique_ptr<BugReport>>::iterator;
using const_iterator = SmallVectorImpl<std::unique_ptr<BugReport>>::const_iterator;

... and move it closer to the usage point, right above begin() / end(). WDYT?

This revision is now accepted and ready to land.Sep 2 2019, 2:04 AM
gribozavr added inline comments.Sep 2 2019, 3:04 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
460 ↗(On Diff #218177)

Could even do away with all these typedefs, and four begin/end overloads, and replace everything with:

ArrayRef<std::unique_ptr<BugReport>> Reports() const;

I believe we don't even need a non-const overload since unique_ptr allows mutations regardless.

460 ↗(On Diff #218177)

BugReports would be a better name.

Szelethus accepted this revision.Sep 3 2019, 1:08 PM

Other then the things @gribozavr mentioned, LGTM.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
458 ↗(On Diff #218177)

Prefer using.

NoQ updated this revision to Diff 219011.Sep 5 2019, 5:30 PM

Fxd, thanks!

NoQ marked 4 inline comments as done.Sep 5 2019, 5:30 PM
gribozavr accepted this revision.Sep 6 2019, 3:51 AM

Still LGTM, just some nitpicks to replace iterator usage with index-based accesses (which I believe is simpler).

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
569 ↗(On Diff #219011)

getReports()[0] ?

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2803 ↗(On Diff #219011)
assert(!EQ.getReports().empty());
const BugType& BT = EQ.getReports()[0]->getBugType();

?

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
3139 ↗(On Diff #219011)

getReports()[0] ?

NoQ marked 5 inline comments as done.Sep 9 2019, 1:33 PM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
569 ↗(On Diff #219011)

Mm right :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2019, 1:40 PM