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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
