<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Sep 19, 2011, at 11:27 PM, Ted Kremenek wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Sep 19, 2011, at 6:51 PM, Anna Zaks wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; ">+ FullSourceLoc genLocation(const ParentMap *PM=0) const;<br>+ PathDiagnosticRange genRange(const ParentMap *PM=0) const;</span></blockquote></div><div><br></div>Hi Anna,<div><br><div>Instead of passing the ParentMap, how about having genLocation() lazily query LocationContext for one? That way it only gets created when we need it.</div></div><div><br></div><div>Actually, it it looks like we are still using the LC member variable in genRange(), instead of the passed ParentMap:</div><div><br></div><div><div> case Stmt::ObjCForCollectionStmtClass: {</div><div> SourceLocation L = getValidSourceLocation(S, LC->getParentMap());</div><div> return SourceRange(L, L);</div></div><div><br></div><div>Since LocationContext isn't guaranteed to live for the lifetime of PathDiagnosticLocation, I suggest:</div><div><br></div><div>1) Remove the LC member variable (fix potential use-after-free).</div></div></blockquote><div><br></div>There was still a dependency remaining. Removed in r140146. </div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>2) Pass (possibly null?) LocationContext * to genLocation() and genRange()</div></div></blockquote><div><br></div>Thanks for catching this! LocationContext's constructor is protected so we cannot construct one from AnalysisContext directly (passing ParentMap would solve the issue, I forgot that it's constructed on demand). We could require either LocationContextManager or AnalysisManager to be passed in by the users that do not have LocationContext. AnalysisManager is probably better since that's what is passed to the checkers.</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>3) Lazily query that LocationContext for a ParentMap as needed. LocationContext (or really, AnalysisContext) will make sure the ParentMap is constructed once.</div><div><br></div></div></blockquote><div><br></div>Will do.</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>What do you think?</div><div><br></div><div>Cheers,</div><div>Ted</div></div></blockquote></div><br></body></html>