Looks great. Yes, we could use NULL to replace the GuardScope.
2010/9/23 Marcin Świderski <[email protected]> > GuardScope was of course being modified... I've removed it. I've also > replaced in const_iterator iterator with unsigned (as Zhongxing sugested) > and fixed problem with iterator pointing to end of scope (should point to > parent instead). > > New patch waiting for approval. > > W dniu 23 września 2010 07:55 użytkownik Marcin Świderski < > [email protected]> napisał: > > 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
