thanks for bring this point up. Actually i have realized this when i was implementing this, but forgot about it when i review this patch. In my patch, i have used an unsinged integer as the iterator, which is a kind of stable iterator.
On 9/23/10, Ted Kremenek <[email protected]> wrote: > > 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? _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
