Page MenuHomePhabricator

[Analyzer] Store BodyFarm in std::unique_ptr
ClosedPublic

Authored by george.karpenkov on Oct 23 2017, 6:30 PM.

Details

Summary

While by using .get() method we don't get the full protection offered by std::unique_ptr, given that two bugs were already encountered (http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/9065 and omitted nullptr assignment), using std::unique_ptr seems strictly better than raw pointer for all practical purposes.

Diff Detail

Repository
rL LLVM

Event Timeline

lichray added inline comments.
lib/Analysis/AnalysisDeclContext.cpp
606 ↗(On Diff #119989)

Why having empty user-defined dtor? This class has no vtbl; deleting the dtor declaration in AnalysisDeclContext.h should work.

lichray accepted this revision.Oct 24 2017, 1:18 PM

LGTM

This revision is now accepted and ready to land.Oct 24 2017, 1:18 PM
alexfh added a subscriber: alexfh.Oct 24 2017, 4:15 PM
alexfh added inline comments.
include/clang/Analysis/AnalysisDeclContext.h
424 ↗(On Diff #120109)

BdyFrm is somewhat cryptic. Maybe Farm, Bodies or something else that is not so weirdly abbreviated?

include/clang/Analysis/AnalysisDeclContext.h
424 ↗(On Diff #120109)

But that's just forced on us by the naming convention, isn't it? I think other names are worse though: they don't tell us which class we can look at to figure out what is it doing.

This revision was automatically updated to reflect the committed changes.
alexfh added inline comments.Oct 25 2017, 8:38 AM
include/clang/Analysis/AnalysisDeclContext.h
424 ↗(On Diff #120109)

LLVM Coding Standards doesn't endorse this kind of an abbreviation, on the opposite:
"We cannot stress enough how important it is to use descriptive names. ... Avoid abbreviations unless they are well known." (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly). Neither "Bdy" nor "Frm" are well known or otherwise clear to the reader. "Frm" could also stand for "Form" or "Frame", but in these cases it would also be confusing.

If you consider shorter names ("Bodies" or "Farm") non-informative, we can go the other direction, e.g. "FunctionBodyFarm".

include/clang/Analysis/AnalysisDeclContext.h
424 ↗(On Diff #120109)

What I've meant to say is that I think a good way to write it would be to use unique_ptr<BodyFarm> bodyFarm, or even better unique_ptr<BodyFarm> m_bodyFarm, which is sadly not allowed. We already have a good informative name: the name of the class, and due to the convention we have to modify it :(

I agree with Alexander here. The LLVM naming convention is not going to change and we should err on the side of using descriptive names. How about "FunctionBodyFarm"?

@dcoughlin OK, I'll commit that [I assuming not doing the review would be fine here]

@dcoughlin OK, I'll commit that [I assuming not doing the review would be fine here]

Yes, thanks!