<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Looks good to me! One nit:<div><br></div><div><div>+ VarDecl* operator*() const { return Scope->Vars[VarIter - 1]; }</div><div>+ VarDecl* const* operator->() const { return &Scope->Vars[VarIter - 1]; }</div><div>+</div><div><br></div><div>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.</div><div><br></div><div>Otherwise, looks great.</div><div><br><div><div><div>On Sep 22, 2010, at 11:58 PM, Marcin Świderski wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">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">marcin.sfider@gmail.com</a>></span> napisał:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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 class="im">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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 class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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 class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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 class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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><br></div>
<span><cfg-local-scope.patch></span></blockquote></div><br></div></div></div></body></html>