This is an archive of the discontinued LLVM Phabricator instance.

Add PostStmt callback for ArraySubscriptExpr
ClosedPublic

Authored by jan on Sep 28 2016, 2:01 AM.

Diff Detail

Event Timeline

jan updated this revision to Diff 72781.Sep 28 2016, 2:01 AM
jan retitled this revision from to runCheckersForPostStmt for ArraySubscriptExpr.
jan updated this object.
jan added a reviewer: a.sidorin.
jan changed the visibility from "Public (No Login Required)" to "All Users".
a.sidorin added inline comments.Sep 28 2016, 2:15 AM
lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
28

I'd like to see Stmt-related options together (PreStmt<Type1>, PostStmt<Type1>, PreStmt<Type2>, PostStmt<Type2>). Not sure, however. Anna, Artem, what do you think? Is there any style recommendation for that?

lib/StaticAnalyzer/Core/ExprEngine.cpp
1990

You should use a temporary node set for PreStmt and then use Dst for PostStmt. Please see an example in ExprEngine.cpp:1213.

a.sidorin requested changes to this revision.Sep 28 2016, 2:16 AM
a.sidorin edited edge metadata.
This revision now requires changes to proceed.Sep 28 2016, 2:16 AM
jan updated this revision to Diff 72807.Sep 28 2016, 5:22 AM
jan updated this object.
jan edited edge metadata.
jan changed the visibility from "All Users" to "Public (No Login Required)".
jan updated this revision to Diff 72811.Sep 28 2016, 6:05 AM
jan updated this object.
jan edited edge metadata.
jan marked an inline comment as done.
a.sidorin added inline comments.Sep 28 2016, 6:14 AM
lib/StaticAnalyzer/Core/ExprEngine.cpp
1982

auto *Node

jan updated this revision to Diff 72815.Sep 28 2016, 6:19 AM
jan updated this object.
a.sidorin accepted this revision.Sep 28 2016, 6:21 AM
a.sidorin edited edge metadata.

Looks good. Anna/Artem?

This revision is now accepted and ready to land.Sep 28 2016, 6:21 AM
NoQ accepted this revision.Sep 28 2016, 9:14 AM
NoQ edited edge metadata.

So we did run checkers for PreStmt but forgot PostStmt? Nice! Thanks for noticing and fixing.

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
28–31

I agree that having one place to change stuff is better than having two places to change stuff, so i'd also prefer pre-post-pre-pos-...-pre-post. In the long term, when this list becomes very long, i'd still suggest we only make PreStmt<Stmt> and PostStmt<Stmt> and construct some dynamic switch in the implementation.

lib/StaticAnalyzer/Core/ExprEngine.cpp
1982

Indent seems to have moved.

zaks.anna edited edge metadata.Sep 28 2016, 9:24 AM

Looks good overall.

Please, include a more elaborate commit message, mentioning that you are adding a new callback.

lib/StaticAnalyzer/Core/ExprEngine.cpp
1974

Let's keep the prefix of the old name for "checkerPreStmt" as it is more explicit - those are the states we get by running the checkers registered for preStmt. This is also more consistent with naming used elsewhere.

(Not sure what to do about capitalization though. Looks like it's already inconsistent in this file. The LLVM coding style prefers lower letters, but this part of the analyzer has been written before that went into effect.)

jan updated this revision to Diff 73264.Oct 3 2016, 5:48 AM
jan retitled this revision from runCheckersForPostStmt for ArraySubscriptExpr to Add PostStmt callback for ArraySubscriptExpr.
jan edited edge metadata.
jan updated this revision to Diff 73266.Oct 3 2016, 5:57 AM

Seems like all comments were addressed. Thank you!

zaks.anna accepted this revision.Oct 3 2016, 10:19 AM
zaks.anna edited edge metadata.

Thank you!

Do you have commit access or should we commit?

jan added a comment.Oct 3 2016, 10:22 AM

Please go ahead and commit. I do not have access.
Thanks

This revision was automatically updated to reflect the committed changes.