2010/9/23 Ted Kremenek <[email protected]> > > 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. >
We could use an unsigned integer to record the relative position within the vector here. It is less heavy weight and simpler.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
