[cfe-commits] [PATCH] LocalScope

[Marcin Świderski]

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/b0bf562f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfg-local-scope.patch
Type: application/octet-stream
Size: 14246 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100923/b0bf562f/attachment.obj>