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

[Антон Ярцев]

  Comment just to submit inline comments


================
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.
----------------
Anna Zaks wrote:
> 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..
I don't particulary like the idea to keep traits into the PointerEscapeKind enum.
They are related, but at the same time they differ in their meaning and nature.
Escape kinds describe reasons of a pointer escapes, kinds are mutually exclusive.
Traits currently describe invalidation traits (preserve contents, invalidate 
indirect regions, etc.). Any number of traits may be attached to a region at the
same time.
Escape kinds and traits are independent - each escape kind may come with any set 
of traits.

Foud the reason for the separate ConstEscape callback in out mail correspondence:
Re: [PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators
29.03.2013 4:00
>>>>>>>> Anton, I think handling this similarly to pointer escape is the best.
>>>>>>>> However, I am concerned that if we simply extend pointer escape to 
>>>>>>>> trigger on another "Kind" of escape, all the other users of 
>>>>>>>> pointerEscape will need to know about it (and do nothing for this kind 
>>>>>>>> of escape). How about a new checker callback, which will rely on the 
>>>>>>>> same core code as _checkPointerEscape? This way the checker developers
>>>>>>>> would not need to know about this special case, and we can reuse all
>>>>>>>> the code that determines when the escape should be triggered.
>>>>>> Anna, As for me it would be better to leave the single callback as this 
>>>>>> is just another type of pointer escape, if I understand correctly. 
>>>>>> Have no other arguments for this :)
>>>>>> In addition, the "Kind" approach is relatively new, so hopefully a few 
>>>>>> users of pointerEscape be affected. 
>>>> Anton, The main concern is that every user of the callback will now have 
>>>> to check if the kind is ConstPointer (or whatever we call it, maybe 
>>>> multiple kinds..) and do nothing in that case. So every user will need to 
>>>> know about this special case and handle it specially.
>> Anton, I've committed the new callback. Looks like no email was sent out.. 
>> So just look at r178310.


================
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:377
@@ -374,3 +376,3 @@
                               PointerEscapeKind Kind,
-                              bool IsConst = false);
+                              RegionAndSymHandlingTraits *HTraits = NULL);
 
----------------
Anna Zaks wrote:
> When does it make sense for traits to be missing? (Why the NULL initialization? Also, I think it should be '0', not "NULL")
Currently pointer escape on bind do not deal with traits ( ExprEngine.cpp, processPointerEscapedOnBind() ), NULL initialization just allows to leave the code of processPointerEscapedOnBind() unchanged. This is the only reason for NULL initialization. Do you think NULL initialization should be removed?

Changed "NULL" to "0". Searched LLVM code standard for "0" vs "NULL", found nothing. What is wrong with "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,
----------------
Anna Zaks wrote:
>  Remove white space, include the param comment.
Done!

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

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

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

================
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 
----------------
Anna Zaks wrote:
> 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).
Removed for now.


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