<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 17, 2013, at 10:52 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Small comments:</div><div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">@@ -119,6 +120,62 @@ PathDiagnostic::PathDiagnostic(const Dec<br> UniqueingDecl(DeclToUnique),<br> path(pathImpl) {}<br><br>+static PathDiagnosticCallPiece *<br>+getFirstStackedCallToHeaderFile(PathDiagnosticCallPiece *CP,<br>+ const SourceManager &SMgr) {<br>+ assert(SMgr.isFromMainFile(CP->callEnter.asLocation()) &&<br>+ "The call piece should be in the main file.");<br>+<br>+ // Check if CP represents a path through a function outside of the main file.<br>+ if (!SMgr.isFromMainFile(CP->callEnterWithin.asLocation()))<br>+ return CP;<br>+<br>+ // Check if the last piece in the callee path is a call to a function outside<br>+ // of the main file.<br>+ const PathPieces &Path = CP->path;<br>+ if (PathDiagnosticCallPiece *CPInner =<br>+ dyn_cast<PathDiagnosticCallPiece>(Path.back())) {<br>+ return getFirstStackedCallToHeaderFile(CPInner, SMgr);<br>+ }<br></div></blockquote><div dir="auto"><br></div><div dir="auto">The inner path might be empty for bugs that have path pruning turned off. I don't think we have many of those, but it's worth checking.</div><div dir="auto"><br></div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+ // Otherwise, the last piece is in the main file.<br>+ return 0;<br>+}<br>+<br>+void PathDiagnostic::resetDiagnosticLocationToMainFile() {<br>+ if (path.empty())<br>+ return;<br>+<br>+ PathDiagnosticPiece *LastP = path.back().getPtr();<br>+ const SourceManager &SMgr = LastP->getLocation().getManager();<br>+ assert(LastP);<br></div></blockquote><div><br></div><div>You're using LastP before asserting it here.</div><div><br></div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+ // We only need to check if the report ends inside headers, if the last piece<br>+ // is a call piece.<br>+ if (PathDiagnosticCallPiece *CP = dyn_cast<PathDiagnosticCallPiece>(LastP)) {<br>+ CP = getFirstStackedCallToHeaderFile(CP, SMgr);<br>+ if (CP) {<br>+ // Mark the piece.<br>+ CP->setAsLastInMainSourceFile();<br>+<br>+ // Update the path diagnostic message.<br>+ const NamedDecl *ND = dyn_cast<NamedDecl>(CP->getCallee());<br>+ if (ND) {<br>+ SmallString<200> buf;<br>+ llvm::raw_svector_ostream os(buf);<br>+ os << " (within a call to " << ND->getDeclName().getAsString() << ")";<br>+ appendToDesc(os.str());<br>+ }<br></div></blockquote><div><br></div><div>Very recently this became just "<< ND->getDeclName()", which is a bit cleaner. This should probably be single-quoted, too, since this will look weird for, say, copy constructors:</div><div><br></div><div>"(within a call to ArrayRef)"</div><div>vs.</div><div>"(within a call to 'ArrayRef')"</div><div><br></div><div>...but really we need to refactor all our function/method pretty-printing so that we get nice names like "copy constructor for ArrayRef" and "-fetchEverything:delegate:".</div><div><br></div><div>Also, it might be worth commenting on the<span class="Apple-converted-space"> </span><i>non-</i>NamedDecl case of a block, which we don't expect to happen because people don't usually define blocks in header files. (But they could, so we shouldn't assert.)</div></div></div></blockquote>We would just not print anything. We don't assert..<br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div><br></div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+ // Reset the report containing declaration and location.<br>+ DeclWithIssue = CP->getCaller();<br>+ Loc = CP->getLocation();<br>+ <br>+ return;<br>+ }<br>+ }<br>+}<br>+<br>void PathDiagnosticConsumer::anchor() { }<br><br>PathDiagnosticConsumer::~PathDiagnosticConsumer() {<br><br>Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp<br>URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=182058&r1=182057&r2=182058&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=182058&r1=182057&r2=182058&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)<br>+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Thu May 16 17:30:45 2013<br>@@ -215,13 +215,18 @@ static void ReportEvent(raw_ostream &o,<br> const SourceManager &SM,<br> const LangOptions &LangOpts,<br> unsigned indent,<br>- unsigned depth) {<br>+ unsigned depth,<br>+ bool isKeyEvent = false) {<br><br> Indent(o, indent) << "<dict>\n";<br> ++indent;<br><br> Indent(o, indent) << "<key>kind</key><string>event</string>\n";<br><br>+ if (isKeyEvent) {<br>+ Indent(o, indent) << "<key>key_event</key><string>YES</string>\n";<br>+ }<br></div></blockquote><div><br></div><div>The best-practices way to do this is <true/> rather than <string>YES</string>.</div><div><br></div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"> // Output the location.<br> FullSourceLoc L = P.getLocation().asLocation();<br><br>@@ -270,7 +275,8 @@ static void ReportPiece(raw_ostream &o,<br> const LangOptions &LangOpts,<br> unsigned indent,<br> unsigned depth,<br>- bool includeControlFlow);<br>+ bool includeControlFlow,<br>+ bool isKeyEvent = false);<br><br>static void ReportCall(raw_ostream &o,<br> const PathDiagnosticCallPiece &P,<br>@@ -283,7 +289,8 @@ static void ReportCall(raw_ostream &o,<br> P.getCallEnterEvent(); <br><br> if (callEnter)<br>- ReportPiece(o, *callEnter, FM, SM, LangOpts, indent, depth, true);<br>+ ReportPiece(o, *callEnter, FM, SM, LangOpts, indent, depth, true,<br>+ P.isLastInMainSourceFile());<br><br> IntrusiveRefCntPtr<PathDiagnosticEventPiece> callEnterWithinCaller =<br> P.getCallEnterWithinCallerEvent();<br>@@ -331,7 +338,8 @@ static void ReportPiece(raw_ostream &o,<br> const LangOptions &LangOpts,<br> unsigned indent,<br> unsigned depth,<br>- bool includeControlFlow) {<br>+ bool includeControlFlow,<br>+ bool isKeyEvent) {<br> switch (P.getKind()) {<br> case PathDiagnosticPiece::ControlFlow:<br> if (includeControlFlow)<br>@@ -344,7 +352,7 @@ static void ReportPiece(raw_ostream &o,<br> break;<br> case PathDiagnosticPiece::Event:<br> ReportEvent(o, cast<PathDiagnosticSpotPiece>(P), FM, SM, LangOpts,<br>- indent, depth);<br>+ indent, depth, isKeyEvent);<br> break;<br> case PathDiagnosticPiece::Macro:<br> ReportMacro(o, cast<PathDiagnosticMacroPiece>(P), FM, SM, LangOpts,</div></blockquote></div></div></blockquote></div><br></body></html>