[cfe-commits] [PATCH] LocalScope

[Marcin Świderski]

Hi

Thanks for comments. Answers inline:

W dniu 23 września 2010 04:24 użytkownik Ted Kremenek
<kremenek at apple.com>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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100923/08ee7638/attachment.html>