r182058 - [analyzer] Add an option to use the last location in the main source file as the report location.

[Jordan Rose]

Small comments:

> @@ -119,6 +120,62 @@ PathDiagnostic::PathDiagnostic(const Dec
>     UniqueingDecl(DeclToUnique),
>     path(pathImpl) {}
> 
> +static PathDiagnosticCallPiece *
> +getFirstStackedCallToHeaderFile(PathDiagnosticCallPiece *CP,
> +                                const SourceManager &SMgr) {
> +  assert(SMgr.isFromMainFile(CP->callEnter.asLocation()) &&
> +         "The call piece should be in the main file.");
> +
> +  // Check if CP represents a path through a function outside of the main file.
> +  if (!SMgr.isFromMainFile(CP->callEnterWithin.asLocation()))
> +    return CP;
> +
> +  // Check if the last piece in the callee path is a call to a function outside
> +  // of the main file.
> +  const PathPieces &Path = CP->path;
> +  if (PathDiagnosticCallPiece *CPInner =
> +      dyn_cast<PathDiagnosticCallPiece>(Path.back())) {
> +    return getFirstStackedCallToHeaderFile(CPInner, SMgr);
> +  }

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.


> +  // Otherwise, the last piece is in the main file.
> +  return 0;
> +}
> +
> +void PathDiagnostic::resetDiagnosticLocationToMainFile() {
> +  if (path.empty())
> +    return;
> +
> +  PathDiagnosticPiece *LastP = path.back().getPtr();
> +  const SourceManager &SMgr = LastP->getLocation().getManager();
> +  assert(LastP);

You're using LastP before asserting it here.


> +  // We only need to check if the report ends inside headers, if the last piece
> +  // is a call piece.
> +  if (PathDiagnosticCallPiece *CP = dyn_cast<PathDiagnosticCallPiece>(LastP)) {
> +    CP = getFirstStackedCallToHeaderFile(CP, SMgr);
> +    if (CP) {
> +      // Mark the piece.
> +       CP->setAsLastInMainSourceFile();
> +
> +      // Update the path diagnostic message.
> +      const NamedDecl *ND = dyn_cast<NamedDecl>(CP->getCallee());
> +      if (ND) {
> +        SmallString<200> buf;
> +        llvm::raw_svector_ostream os(buf);
> +        os << " (within a call to " << ND->getDeclName().getAsString() << ")";
> +        appendToDesc(os.str());
> +      }

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:

"(within a call to ArrayRef)"
vs.
"(within a call to 'ArrayRef')"

...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:".

Also, it might be worth commenting on the non-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.)


> +      // Reset the report containing declaration and location.
> +      DeclWithIssue = CP->getCaller();
> +      Loc = CP->getLocation();
> +      
> +      return;
> +    }
> +  }
> +}
> +
> void PathDiagnosticConsumer::anchor() { }
> 
> PathDiagnosticConsumer::~PathDiagnosticConsumer() {
> 
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=182058&r1=182057&r2=182058&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Thu May 16 17:30:45 2013
> @@ -215,13 +215,18 @@ static void ReportEvent(raw_ostream &o,
>                         const SourceManager &SM,
>                         const LangOptions &LangOpts,
>                         unsigned indent,
> -                        unsigned depth) {
> +                        unsigned depth,
> +                        bool isKeyEvent = false) {
> 
>   Indent(o, indent) << "<dict>\n";
>   ++indent;
> 
>   Indent(o, indent) << "<key>kind</key><string>event</string>\n";
> 
> +  if (isKeyEvent) {
> +    Indent(o, indent) << "<key>key_event</key><string>YES</string>\n";
> +  }

The best-practices way to do this is <true/> rather than <string>YES</string>.


>   // Output the location.
>   FullSourceLoc L = P.getLocation().asLocation();
> 
> @@ -270,7 +275,8 @@ static void ReportPiece(raw_ostream &o,
>                         const LangOptions &LangOpts,
>                         unsigned indent,
>                         unsigned depth,
> -                        bool includeControlFlow);
> +                        bool includeControlFlow,
> +                        bool isKeyEvent = false);
> 
> static void ReportCall(raw_ostream &o,
>                        const PathDiagnosticCallPiece &P,
> @@ -283,7 +289,8 @@ static void ReportCall(raw_ostream &o,
>     P.getCallEnterEvent();  
> 
>   if (callEnter)
> -    ReportPiece(o, *callEnter, FM, SM, LangOpts, indent, depth, true);
> +    ReportPiece(o, *callEnter, FM, SM, LangOpts, indent, depth, true,
> +                P.isLastInMainSourceFile());
> 
>   IntrusiveRefCntPtr<PathDiagnosticEventPiece> callEnterWithinCaller =
>     P.getCallEnterWithinCallerEvent();
> @@ -331,7 +338,8 @@ static void ReportPiece(raw_ostream &o,
>                         const LangOptions &LangOpts,
>                         unsigned indent,
>                         unsigned depth,
> -                        bool includeControlFlow) {
> +                        bool includeControlFlow,
> +                        bool isKeyEvent) {
>   switch (P.getKind()) {
>     case PathDiagnosticPiece::ControlFlow:
>       if (includeControlFlow)
> @@ -344,7 +352,7 @@ static void ReportPiece(raw_ostream &o,
>       break;
>     case PathDiagnosticPiece::Event:
>       ReportEvent(o, cast<PathDiagnosticSpotPiece>(P), FM, SM, LangOpts,
> -                  indent, depth);
> +                  indent, depth, isKeyEvent);
>       break;
>     case PathDiagnosticPiece::Macro:
>       ReportMacro(o, cast<PathDiagnosticMacroPiece>(P), FM, SM, LangOpts,
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130517/d516a215/attachment.html>