<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><blockquote type="cite" style="font-size: 15px; ">Bring it on! (: <br></blockquote><div style="font-size: 15px; "><br></div><span style="font-size: 15px; ">*cracks knuckles*</span><div style="font-size: 15px; "><br></div><div><span style="font-size: 15px; ">Sorry I haven't been following your thread so closely; been drawn away by other things. I agree that adding the </span><span style="font-size: 15px;">PointerEscapeKind enum is the right way to go, and the patch is pretty straightforward. So, a lot of spot comments...</span></div><div><span style="font-size: 15px;"><br></span></div><div><span style="font-size: 15px;"><br></span></div><div><span style="font-size: 15px;">+ const enum PointerEscapeKind Kind) {</span></div><div><span style="font-size: 15px;"><br></span></div><div><span style="font-size: 15px;">No "const" needed for value types, and conventionally no "enum" in LLVM-style C++. This shows up in a couple of places.</span></div><div><span style="font-size: 15px;"><br></span></div><div><span style="font-size: 15px;"><br></span></div><div><span style="font-size: 15px; "><div>+/// \brief Describes the different reasons a pointer escapes</div><div>+/// during analysis.</div><div>+enum PointerEscapeKind {</div><div>+ /// A pointer escapes due to binding its value to a location,</div><div>+ /// preventing further reasoning on the pointer.</div><div>+ PSK_EscapeOnBind = 1,</div><div><br></div><div>This makes it sound like this happens for <i>any</i> location, which isn't true. How about "a location the analyzer can't track"?</div><div><br></div><div><br></div><div>+ /// The pointer has been passed to a function call directly.</div><div>+ PSK_DirectEscapeOnCall,</div><div>+</div><div>+ /// The pointer has been passed to a function indirectly.</div><div>+ /// For example, the pointer is accessable through an</div><div>+ /// argument to a function.</div><div>+ PSK_IndirectEscapeOnCall,</div><div><br></div><div>Typo: "accessable"</div><div><br></div><div><br></div><div>+ ///The reason for pointer escape is unknown. For example, a checker</div><div>+ ///invalidates a region and intends the invalidation to cause a</div><div>+ ///pointer escape event.</div><div><div>+ PSK_EscapeOther</div></div><div><br></div><div>Missing spaces after the slashes. Also, I feel like this is too explicit—if a region is invalidated and the checker writer doesn't <i>think</i> about whether the contents escape, this still happens. How about "For example, a region containing this pointer is invalidated."?</div><div><br></div><div>+};</div><div><br></div><div>(I do want to say "nice comments", though! Thank you for putting in the time to document everything.)</div><div><br></div><div><br></div><div><div>+ /// \param Kind The kind of escape the pointers have undergone.</div></div><div><br></div><div>Clever use of English. Simpler: "How the symbols have escaped."</div><div><br></div><div><br></div><div><div> ProgramStateRef checkPointerEscape(ProgramStateRef State,</div><div>- const InvalidatedSymbols &Escaped,</div><div>- const CallEvent *Call) const {</div><div>+ const InvalidatedSymbols &Escaped,</div><div>+ const CallEvent *Call,</div><div>+ const enum PointerEscapeKind Kind) const {</div></div><div><br></div><div>Please don't change all the indentation; before the parameters lined up, now they don't. I realize that the last line didn't fit, but so far in the analyzer we've been just tweaking that one line. If you want to change all the lines, please put the first parameter on its own line and indent everything twice only.</div><div><br></div><div>...although the removal of "const" and "enum" may make this short enough to fit.</div><div><br></div><div><br></div><div><div><div>- if (Call && doesNotFreeMemory(Call, State))</div><div>+ if ((Kind == PSK_DirectEscapeOnCall ||</div><div>+ Kind == PSK_IndirectEscapeOnCall) &&</div><div>+ doesNotFreeMemory(Call, State)) {</div></div></div><div><br></div><div>This is not correct. Before, this branch was only taken if the Kind is PSK_DirectEscapeOnCall. Indirect escapes can still possibly free memory (although it's unlikely).</div><div><br></div><div><br></div><div><div>- if (Call && guaranteedNotToCloseFile(*Call))</div><div>+ if ((Kind == PSK_DirectEscapeOnCall ||</div><div>+ Kind == PSK_IndirectEscapeOnCall) &&</div><div>+ guaranteedNotToCloseFile(*Call)) {</div></div><div><br></div><div>Ditto.</div><div><br></div><div><br></div><div><div>+ assert((Kind == PSK_DirectEscapeOnCall ||</div><div>+ Kind == PSK_IndirectEscapeOnCall) ?</div><div>+ Call != NULL : true);</div></div><div><br></div><div>What Anna said about implication.</div><div><br></div><div><br></div><div><div>- EscapedSymbols,</div><div>- /*CallEvent*/ 0);</div><div>+ EscapedSymbols,</div><div>+ /*CallEvent*/ 0,</div><div>+ PSK_EscapeOnBind);</div></div><div><br></div><div><div>- *Invalidated, 0);</div><div>+ *Invalidated,</div><div>+ 0,</div><div>+ PSK_EscapeOther);</div></div><div><br></div><div>These <i>do</i> fit in 80 columns. Please do not reformat arbitrarily.</div><div><br></div><div>I'll review the malloc patch in a separate e-mail.</div></span></div></body></html>