<br><br>
<div class="gmail_quote">2010/9/23 Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span><br>
<blockquote style="BORDER-LEFT: #ccc 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">
<div>
<div></div>
<div class="h5"><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>+  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>
</blockquote>
<div> </div>
<div>We could use an unsigned integer to record the relative position within the vector here. It is less heavy weight and simpler.</div></div>