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. >
cfg-local-scope.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
