[cfe-commits] [PATCH] LocalScope

[Zhongxing Xu]

thanks for bring this point up. Actually i have realized this when i
was implementing this, but forgot about it when i review this patch.
In my patch, i have used an unsinged integer as the iterator, which is
a kind of stable iterator.

On 9/23/10, Ted Kremenek <kremenek at apple.com> wrote:
>
> 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).
>
> +  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.
>
> +public:
> +  LocalScope(const_iterator P)
> +      : Vars()
> +      , Prev(P) {}
>
> Since this is the only public constructor of LocalScope, it's worth putting
> in a comment.
>
> Is there a method you plan on adding that will supporting adding to Vars?