Looks great. <br><br>Yes, we could use NULL to replace the GuardScope.<br><br><div class="gmail_quote">2010/9/23 Marcin Świderski <span dir="ltr"><<a href="mailto:marcin.sfider@gmail.com">marcin.sfider@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">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).<div>
<br></div><div>New patch waiting for approval.<br><br><div class="gmail_quote">W dniu 23 września 2010 07:55 użytkownik Marcin Świderski <span dir="ltr"><<a href="mailto:marcin.sfider@gmail.com" target="_blank">marcin.sfider@gmail.com</a>></span> napisał:<div>
<div></div><div class="h5"><br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_quote">Hi</div><div class="gmail_quote"><br></div><div class="gmail_quote">
Thanks for comments. Answers inline:</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">W dniu 23 września 2010 04:24 użytkownik Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> napisał:<div>
<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div><div></div><div><br>
On Sep 21, 2010, at 11:43 AM, Marcin Świderski wrote:<br>
<br>
> Patch adds:<br>
> - LocalScope class with iterator used to pointing into it,<br>
> - fat doxygen comment for LocalScope indended usage,<br>
> - BlockScopePosPair class used for storing jump targets/sources (for: goto, break, continue), that replaces raw CFGBlock pointer used earlier for this purpose.<br>
><br>
> This patch prepares CFGBuilder for adding generation of destructors for objects with automatic storage.<br>
><br>
> Please aprove for commit.<br>
</div></div>> <cfg-local-scope.patch><br>
<br>
Hi Marcin,<br>
<br>
I have a few comments inline:<br>
<br>
+ /// GuardScope - Guard for "valid" invalid iterator. This allows for simpler<br>
+ /// implementation of increment operator for LocalScope::const_iterator.<br>
+ static LocalScope GuardScope;<br>
<br>
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).<br>
<br></blockquote></div><div>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.</div><div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+ LocalScope::const_iterator LoopBeginScopePos = ScopePos;<br>
<br>
I don't think this is safe, given the definition of LocalScope::const_iterator:<br>
<br>
+class LocalScope {<br>
+public:<br>
+ typedef llvm::SmallVector<VarDecl*, 4> AutomaticVarsTy;<br>
+ typedef AutomaticVarsTy::const_reverse_iterator AutomaticVarsIter;<br>
+<br>
+ /// const_iterator - Iterates local scope backwards and jumps to previous<br>
+ /// scope on reaching the begining of currently iterated scope.<br>
+ class const_iterator {<br>
+ const LocalScope* Scope;<br>
+ AutomaticVarsIter VarIter;<br>
<br>
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.<br>
<br></blockquote></div><div>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?</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
+public:<br>
+ LocalScope(const_iterator P)<br>
+ : Vars()<br>
+ , Prev(P) {}<br>
<br>
Since this is the only public constructor of LocalScope, it's worth putting in a comment. </blockquote><div> </div></div><div>I'll add a comment before commiting the patch. </div><div><div><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Is there a method you plan on adding that will supporting adding to Vars?</blockquote></div></div><br><div>Yes, but I left this for next patch, that will handle creating/using LocalScopes.</div>
</blockquote></div></div></div><br></div>
</blockquote></div><br>