[cfe-commits] [PATCH] LocalScope

[Ted Kremenek]

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 <marcin.sfider at gmail.com> napisał:
> 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.
> 
> <cfg-local-scope.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100924/96daab4b/attachment.html>