[cfe-commits] [PATCH] LocalScope

[Zhongxing Xu]

2010/9/23 Ted Kremenek <kremenek at apple.com>

>
> 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.
>

We could use an unsigned integer to record the relative position within the
vector here. It is less heavy weight and simpler.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100923/376a4fde/attachment.html>