[PATCH] D41151: [analyzer] Adding LoopContext and improve loop modeling

[Peter Szecsi via Phabricator via cfe-commits]

szepet updated this revision to Diff 130516.
szepet marked 2 inline comments as done.
szepet added a comment.

First, sorry for this delayed update, however, I was working on this and running this on real projects. I wanted to make sure that this update will be complete enough that this patch would not cause any harm (yes, these features are hidden behind a flag but anyway) and not crashes on edge cases I haven't thought of. The core used the fact that LocationContext only contains StackFramce and BlockInvocation and I aimed to eliminate all these code snippets (more like rewrite).

> This thing is very similar to https://reviews.llvm.org/D19979. Do we really need to create a separate LoopContext or we can reuse ScopeContext instead?



> I guess LoopContext can be treated as a sub-class of ScopeContext. And i don't mind having ScopeContext be split into small distinct sub-classes. Because we're stuck in corner cases for covering all possible scopes, while we're fine with covering only simple scopes (it's better than nothing) and lacking a scope from time to time.

In this patch I left it as it was, so a separate context. I agree with Aleksei that yes, it could be implemented as a ScopeContext and checked if it contains a While/Do/For Statement.
On the other hand, I like the idea more, that we split ScopeContext into small distinct subclasses which would result in a more manageable code.
However, this would require refactoring on ScopeContext as well, in order to make it a great base class (like StmtPoint for ProgramPoint). Then, LoopContext would be its only subclass. So, I do not really see the point of doing these changes right now. I think in this state (when ScopeContext not used by the analyzer as I can see) the LoopContext could live as a separate Context and not make any harm. In case when another Context shows up which is similar LoopContext (e.g. ScopeContext) I would happily refactor it but right now, I do not see the point of this.


https://reviews.llvm.org/D41151

Files:
  include/clang/Analysis/AnalysisDeclContext.h
  include/clang/Analysis/ProgramPoint.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  include/clang/StaticAnalyzer/Core/PathSensitive/LoopUnrolling.h
  lib/Analysis/AnalysisDeclContext.cpp
  lib/StaticAnalyzer/Core/BugReporter.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/CoreEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/loop-unrolling.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41151.130516.patch
Type: text/x-patch
Size: 29632 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180119/0e7b2e1b/attachment-0001.bin>