Page MenuHomePhabricator

[CFG] [analyzer] Add construction context for CXXCtorInitializer.
ClosedPublic

Authored by NoQ on Jan 30 2018, 11:49 AM.

Details

Summary

This patch builds on top of D42672 to add construction contexts for constructors that are initializing CXXCtorInitializer nodes (C::C(int x, struct S s): x(x), s(s) /* <== these little thingines */ {}). Because CXXCtorInitializer is not a Stmt, ConstructionContext needs to be extended to support such contexts. This patch refactors ConstructionContext into a "proper" class with fancy getters (we'd most likely need setters later as well).

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Jan 30 2018, 11:49 AM
include/clang/Analysis/CFG.h
167 ↗(On Diff #132015)

I think we should get<> here instead, to clearly crash on incorrect operations.
I also don't understand what dyn_cast does in such cases: doxygen comment says it returns null, yet the code calls the default empty constructor, which I think is even more confusing.

lib/Analysis/CFG.cpp
1262 ↗(On Diff #132015)

auto *CE?

1263 ↗(On Diff #132015)

Could we have an explicit constructor call here?

NoQ added inline comments.Jan 30 2018, 1:28 PM
include/clang/Analysis/CFG.h
167 ↗(On Diff #132015)

I think we should get<> here instead, to clearly crash on incorrect operations.

Yeah, i guess, it'd eventually evolve into a whole class hierarchy of construction contexts with RTTI and stuff. I'm trying to grow it slowly because for now i don't foresee exactly how much of this we'd need.

Still, to me it seems more natural to ask "hey does this thing have a trigger statement", "oh, so maybe it has an initializer", than to enumerate all possible reasons why it may have a trigger statement or an initializer and match for those. It seems easier for callers to use.

the code calls the default empty constructor

This is an empty constructor for T, where T is in our case CXXCtorInitializer *, so it boils down to returning nullptr.

NoQ updated this revision to Diff 132056.Jan 30 2018, 2:44 PM

Remove the stack of contexts for now. Which, by the way, i forgot to pop in this patch.

NoQ added inline comments.
lib/Analysis/CFG.cpp
1262 ↗(On Diff #132015)

Overwritten in D42719.

1263 ↗(On Diff #132015)

Overwritten in D42719.

dcoughlin added inline comments.Jan 31 2018, 11:46 PM
include/clang/Analysis/CFG.h
184 ↗(On Diff #132056)

Extra semi-colon here.

lib/Analysis/CFG.cpp
4649 ↗(On Diff #132056)

This is pretty fragile if you ever add a third kind. Do you want some kind of a 'Kind' you can switch over and get a warning when you add a new one?

NoQ updated this revision to Diff 132692.Feb 2 2018, 3:06 PM

Rebase. Add direct tests.

NoQ updated this revision to Diff 132693.Feb 2 2018, 3:11 PM

Add an assertion to see if we can always print the construction trigger.

lib/Analysis/CFG.cpp
4649 ↗(On Diff #132056)

Yep, this is indeed likely that i'd end up adding a whole RTTI of construction contexts.
Added a direct assertion for now.

NoQ marked 4 inline comments as done.Feb 2 2018, 3:12 PM
NoQ edited the summary of this revision. (Show Details)Feb 2 2018, 4:15 PM
NoQ updated this revision to Diff 133141.Feb 6 2018, 7:45 PM

Rebase.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 9 2018, 6:19 PM
Closed by commit rL324796: [CFG] Add construction context for constructor initializers. (authored by dergachev, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.