Hi Thanks for comments. Answers inline:
W dniu 23 września 2010 04:24 użytkownik Ted Kremenek <[email protected]>napisał: > > 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). > > GuardScope is never modified. It's just a guard. Won't it make it safe? Creating context for it seams to be a little overkill. > + 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. > > I made special consideration not to allow for iterators to LocalScope to be invalidated. When I create iterator to some LocalScope it won't be modified. But maybe we will discuss this with next patch, as this one is mainly to replace usage of CGFBlock* with BlockScopePosPair and does not contain mechanism of creating/using LocalScopes? > +public: > + LocalScope(const_iterator P) > + : Vars() > + , Prev(P) {} > > Since this is the only public constructor of LocalScope, it's worth putting > in a comment. I'll add a comment before commiting the patch. Is there a method you plan on adding that will supporting adding to Vars? Yes, but I left this for next patch, that will handle creating/using LocalScopes.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
