[PATCH] [Review request][analyzer]Information about special handling for a particular region/symbol.

[Anna Zaks]

  Here is my high-level understanding of what this patch does:

  This patch removes explicit passing around of const-escape vs regular-escape info (region list / flags) by passing around a datastructure that maps regions and symbols to the type of escape they experience. This simplifies the code and would allow to associate more different escape types in the future.

  I've added lower-level comments inline. Also, I don't particularly like the name "RegionAndSymHandlingTraits" (and all the derivative parameter names like "HTraits" or something similar) - it's too generic; unclear what "handling" you are talking about. How about "RegionAndSymbolEscapeTraits"?

  Other than that, I think this looks good and does simplify the code quite a bit.

  So thanks for working on this!
  Anna.


================
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:368-370
@@ -367,3 +367,5 @@
   ///        parameters to the given call.
-  /// \param IsConst Specifies if the pointer is const.
+  /// \param Kind The reason of pointer escape.
+  /// \param HTraits Information about special handling for a particular 
+  ///        region/symbol.
   /// \returns Checkers can modify the state by returning a new one.
----------------
I know this is not new to this patch, but PointerEscapeKind and the traits info are very related...

Would it make sense to extend the kind enum and use it for traits? One benefit I see is that all the info about the escape would be in one place. We would only have one checker callback.. On the other hand, I don't recall why they are different concepts now..

================
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:377
@@ -374,3 +376,3 @@
                               PointerEscapeKind Kind,
-                              bool IsConst = false);
+                              RegionAndSymHandlingTraits *HTraits = NULL);
 
----------------
When does it make sense for traits to be missing? (Why the NULL initialization? Also, I think it should be '0', not "NULL")

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:479
@@ -478,4 +478,3 @@
   /// region invalidation.
-  /// \param[in] IsConst Specifies that the pointer is const.
   ProgramStateRef notifyCheckersOfPointerEscape(
                             ProgramStateRef State,
----------------
 Remove white space, include the param comment.

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1330
@@ +1329,3 @@
+  enum TraitKinds {
+    TK_PreserveContents = 0x1,
+    TK_DoNotInvalidateIndirections = 0x2
----------------
All the different kinds need good documentation. This is a good place for it.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1331
@@ +1330,3 @@
+    TK_PreserveContents = 0x1,
+    TK_DoNotInvalidateIndirections = 0x2
+    // Do not forget to extend StorageTypeForKinds if number of traits exceed 
----------------
This one is not used. We are only adding support for escapes as const in this patch, which is represented by the enum element above (as far as I can tell).

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1327
@@ +1326,3 @@
+      SymTraitsMapConstItTy;
+
+public:
----------------
these type names do not look like they are LLVM's iterator types. Please, look around for examples of more conventional iterator type names.

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1316
@@ +1315,3 @@
+//===----------------------------------------------------------------------===//
+
+/// Information about special handling for a particular region/symbol.
----------------
I am not sure if this should do into this header.. Maybe some of the implementation should go into a cpp file.


http://llvm-reviews.chandlerc.com/D1486