[cfe-commits] [PATCH] LocalScope

[Ted Kremenek]

On Sep 21, 2010, at 11:43 AM, Marcin Świderski wrote:

> Patch adds:
> - LocalScope class with iterator used to pointing into it,
> - fat doxygen comment for LocalScope indended usage,
> - BlockScopePosPair class used for storing jump targets/sources (for: goto, break, continue), that replaces raw CFGBlock pointer used earlier for this purpose.
> 
> This patch prepares CFGBuilder for adding generation of destructors for objects with automatic storage.
> 
> Please aprove for commit. 
> <cfg-local-scope.patch>

Hi Marcin,

I have a few comments inline:

+    /// GuardScope - Guard for "valid" invalid iterator. This allows for simpler
+    /// implementation of increment operator for LocalScope::const_iterator.
+    static LocalScope GuardScope;

Please do not use a static variable.  We should allow for multiple instances of CFGBuilder to safely run concurrently.  If you need to record context, you'll need a context object of some kind (which is the practice we take in the rest of Clang).

+  LocalScope::const_iterator LoopBeginScopePos = ScopePos;

I don't think this is safe, given the definition of LocalScope::const_iterator:

+class LocalScope {
+public:
+  typedef llvm::SmallVector<VarDecl*, 4> AutomaticVarsTy;
+  typedef AutomaticVarsTy::const_reverse_iterator AutomaticVarsIter;
+
+  /// const_iterator - Iterates local scope backwards and jumps to previous
+  /// scope on reaching the begining of currently iterated scope.
+  class const_iterator {
+    const LocalScope* Scope;
+    AutomaticVarsIter VarIter;

Since the const_iterator wraps an AutomaticVarsIter, can't that iterator be invalidated if the underlying SmallVector is resized?  It's hard to tell from this patch if it is safe because LocalScope::Vars is never modified.  I assume it will be, in which case you'll probably need to use a functional data structure, like a singly linked list.  From the definition of LocalScope::const_iterator::operator++(), it looks like you'll just be walking a reverse chain of variables anyway.  A BumpPointerAllocated linked list might work really nicely here, and would be safe from iterator invalidation.

+public:
+  LocalScope(const_iterator P)
+      : Vars()
+      , Prev(P) {}

Since this is the only public constructor of LocalScope, it's worth putting in a comment.

Is there a method you plan on adding that will supporting adding to Vars?