Looks good to me! One nit:
+ VarDecl* operator*() const { return Scope->Vars[VarIter - 1]; }
+ VarDecl* const* operator->() const { return &Scope->Vars[VarIter - 1]; }
+
Could you add a comment somewhere, perhaps where VarIter is declared, that
documents that '0' is specially reserved as a sentinel value. Casually
glancing at this code makes this look like a bug. Even just adding
'assert(VarIter > 0)' might make it clearer.
Otherwise, looks great.
On Sep 22, 2010, at 11:58 PM, Marcin Świderski wrote:
> 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>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
