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

[Anna Zaks]

On May 17, 2013, at 10:52 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> 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.)
We would just not print anything. We don'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/cbc07f2a/attachment.html>