[cfe-commits] [PATCH] LocalScope

[Zhongxing Xu]

Looks great.

Yes, we could use NULL to replace the GuardScope.

2010/9/23 Marcin Świderski <marcin.sfider at gmail.com>

> 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.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100923/02937390/attachment.html>