[PATCH] D26588: Add LocationContext to members of check::RegionChanges

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Hi and welcome to the project!

This patch definitely looks quite complex for a first contribution, so great job at digging through the analyzer internals!

One higher level comment I have is that you should try and split patches whenever possible. For example, in the description, you mention that the patch contains 2 changes:

- extending RegionChanges interface with LocationContext
- adding an easy way to obtain arguments' SVals from StackFrameCtx

Since they are independent changes, please submit them as separate patches. This simplifies the job of the reviewers and anyone who will ever look at this patch in commit history or on phabricator. Also, this ensures that each change has sufficient test coverage. The LLVM Dev policy has a great explanation of the reasoning behind this: http://llvm.org/docs/DeveloperPolicy.html#incremental-development.

How do you plan on testing these changes? The main 2 methods are unit tests and checker regression tests. Since no checker uses these maybe we should only commit them once such checker is added...

I've only started looking at the first change and would like to understand the motivation behind it a bit more.



================
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:325
+                      const CallEvent *Call,
+                      const LocationContext *LCtx) {
+    return ((const CHECKER *) checker)->checkRegionChanges(state, invalidated,
----------------
LocationContext can be obtained by calling CallEvent::getLocationContext(). I do not think that adding another parameter here buys us much. Am I missing something?


================
Comment at: include/clang/StaticAnalyzer/Core/Checker.h:333
+                                       ProgramStateRef state,
+                                       const LocationContext *LCtx) {
+    return ((const CHECKER *) checker)->wantsRegionChangeUpdate(state, LCtx);
----------------
What is the scenario in which you need the context here? The idea is that the checker would be interested in region changes if it is already tracking information in the state. For that check, the information in the state is sufficient. What is your scenario?

Also, For any checker API change, we need to update the CheckerDocumentation.cpp.


https://reviews.llvm.org/D26588