This is an archive of the discontinued LLVM Phabricator instance.

[CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases.
ClosedPublic

Authored by NoQ on Feb 20 2018, 3:34 PM.

Details

Summary

ConstructionContext is moved into a separate translation unit and is separated into multiple classes.

The "old" "raw" ConstructionContext is renamed into ConstructionContextLayer - which corresponds to the idea of building the context gradually layer-by-layer, but it isn't easy to use in the clients. Once CXXConstructExpr is reached, layers that we've gathered so far are transformed into the actual, "new-style" "flat" ConstructionContext, which is put into the CFGConstructor element and has no layers whatsoever (until it actually needs them, eg. aggregate initialization). The new-style ConstructionContext is instead presented as a variety of sub-classes that enumerate different ways of constructing an object in C++. There are 5 (five) of these supported for now, which is around a half of what needs to be supported.

The layer-by-layer buildup process is still a bit weird, but it's pretty functional with all the immutable stuff and pattern-matching, and i'm not seeing any easy alternatives.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 20 2018, 3:34 PM
NoQ updated this revision to Diff 135154.Feb 20 2018, 3:40 PM

Remove unused methods.

This looks great to me Artem! I'll be very curious to see how you extend this to handle initializer lists.

lib/Analysis/CFG.cpp
4737 ↗(On Diff #135154)

Eventually (not now) I think it would be great to include the construction context kind in the printed CFG. This would make it easier to understand at a glance the context.

lib/Analysis/ConstructionContext.cpp
49 ↗(On Diff #135154)

I like how this puts all the messy pattern matching in one place. The follows the general LLVM guidelines of "if it has to be messy, put it all in one place and hide the messiness from everything else".

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
643 ↗(On Diff #135154)

This is much more semantically clear. Thank you!!

dcoughlin accepted this revision.Feb 20 2018, 7:07 PM
This revision is now accepted and ready to land.Feb 20 2018, 7:07 PM
a.sidorin accepted this revision.Feb 21 2018, 3:45 AM
a.sidorin added inline comments.
include/clang/Analysis/ConstructionContext.h
119 ↗(On Diff #135154)

Maybe just build()? For me, finalize() strongly associates with Java's broken clean-up mechanism.

NoQ updated this revision to Diff 135480.Feb 22 2018, 11:09 AM

Address comments.

include/clang/Analysis/ConstructionContext.h
119 ↗(On Diff #135154)

Renamed into createFromLayers().

lib/Analysis/CFG.cpp
4737 ↗(On Diff #135154)

Yeah, the only reason i don't do verbose printing is because changes in tests are massive.

lib/Analysis/ConstructionContext.cpp
49 ↗(On Diff #135154)

Actually two places, the other one being findConstructionContext().

NoQ updated this revision to Diff 135667.EditedFeb 23 2018, 11:08 AM

Missing break!~~

This revision was automatically updated to reflect the committed changes.