r179396 - [analyzer] Print a diagnostic note even if the region cannot be printed.

[Jordan Rose]

I know I suggested the "-ing" form, but it actually didn't come out as nice as I thought. Some alternate suggestions inline:


On Apr 12, 2013, at 11:40 , Anna Zaks <ganna at apple.com> wrote:

> Author: zaks
> Date: Fri Apr 12 13:40:27 2013
> New Revision: 179396
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=179396&view=rev
> Log:
> [analyzer] Print a diagnostic note even if the region cannot be printed.
> 
> There are few cases where we can track the region, but cannot print the note,
> which makes the testing limited. (Though, I’ve tested this manually by making
> all regions non-printable.) Even though the applicability is limited now, the enhancement
> will be more relevant as we start tracking more regions.
> 
> Modified:
>    cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
>    cfe/trunk/test/Analysis/inlining/path-notes.cpp
> 
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=179396&r1=179395&r2=179396&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Fri Apr 12 13:40:27 2013
> @@ -511,9 +511,6 @@ PathDiagnosticPiece *FindLastStoreBRVisi
>     }
>   }
> 
> -  if (!R->canPrintPretty())
> -    return 0;
> -
>   // Okay, we've found the binding. Emit an appropriate message.
>   SmallString<256> sbuf;
>   llvm::raw_svector_ostream os(sbuf);
> @@ -525,9 +522,11 @@ PathDiagnosticPiece *FindLastStoreBRVisi
>     const VarRegion *VR = dyn_cast<VarRegion>(R);
> 
>     if (DS) {
> -      action = "initialized to ";
> +      action = R->canPrintPretty() ? "initialized to " :
> +                                     "Initializing to ";
>     } else if (isa<BlockExpr>(S)) {
> -      action = "captured by block as ";
> +      action = R->canPrintPretty() ? "captured by block as " :
> +                                     "Capturing by block as ";

For some reason this isn't quite grammatical; in this case "Captured by block as " doesn't seem so bad. This will be very rare in practice, though, because blocks can only capture variables, meaning this will only affect blocks capturing null references. (I don't think we'll have garbage references.)



>       if (VR) {
>         // See if we can get the BlockVarRegion.
>         ProgramStateRef State = StoreSite->getState();
> @@ -545,9 +544,11 @@ PathDiagnosticPiece *FindLastStoreBRVisi
>     }
> 
>     if (action) {
> -      R->printPretty(os);
> -      os << " ";
> -      
> +      if (R->canPrintPretty()) {
> +        R->printPretty(os);
> +        os << " ";
> +      }
> +
>       if (V.getAs<loc::ConcreteInt>()) {
>         bool b = false;
>         if (R->isBoundable()) {
> @@ -569,14 +570,18 @@ PathDiagnosticPiece *FindLastStoreBRVisi
>         if (V.isUndef()) {
>           if (isa<VarRegion>(R)) {
>             const VarDecl *VD = cast<VarDecl>(DS->getSingleDecl());
> -            if (VD->getInit())
> -              os << "initialized to a garbage value";
> -            else
> -              os << "declared without an initial value";
> +            if (VD->getInit()) {
> +              os << (R->canPrintPretty() ? "initialized" : "Initializing")
> +                 << " to a garbage value";

We could use 'action' here, which already has "Initializing to" at this point.


> +            } else {
> +              os << (R->canPrintPretty() ? "declared" : "Declaring")
> +                 << " without an initial value";
> +            }
>           }
>         }
>         else {
> -          os << "initialized here";
> +          os << (R->canPrintPretty() ? "initialized" : "Initializing")
> +             << " here";

"Initialized here" sounds better to me, though neither of them are great.


>         }
>       }
>     }
> @@ -602,8 +607,11 @@ PathDiagnosticPiece *FindLastStoreBRVisi
> 
>       // Printed parameter indexes are 1-based, not 0-based.
>       unsigned Idx = Param->getFunctionScopeIndex() + 1;
> -      os << " via " << Idx << llvm::getOrdinalSuffix(Idx) << " parameter ";
> -      R->printPretty(os);
> +      os << " via " << Idx << llvm::getOrdinalSuffix(Idx) << " parameter";
> +      if (R->canPrintPretty()) {
> +        os << " ";
> +        R->printPretty(os);
> +      }

This is much nicer than giving up. :-)


>     }
>   }
> 
> @@ -613,25 +621,28 @@ PathDiagnosticPiece *FindLastStoreBRVisi
>       if (R->isBoundable()) {
>         if (const TypedValueRegion *TR = dyn_cast<TypedValueRegion>(R)) {
>           if (TR->getValueType()->isObjCObjectPointerType()) {
> -            os << "nil object reference stored to ";
> +            os << "nil object reference stored";
>             b = true;
>           }
>         }
>       }
> 
>       if (!b)
> -        os << "Null pointer value stored to ";
> +        os << "Null pointer value stored";
>     }
>     else if (V.isUndef()) {
> -      os << "Uninitialized value stored to ";
> +      os << "Uninitialized value stored";
>     } else if (Optional<nonloc::ConcreteInt> CV =
>                    V.getAs<nonloc::ConcreteInt>()) {
> -      os << "The value " << CV->getValue() << " is assigned to ";
> +      os << "The value " << CV->getValue() << " is assigned";
>     }
>     else
> -      os << "Value assigned to ";
> +      os << "Value assigned";
> 
> -    R->printPretty(os);
> +    if (R->canPrintPretty()) {
> +      os << " to ";
> +      R->printPretty(os);
> +    }

These are where "Storing" sounds much better to me. "Storing nil object reference", "Storing null pointer value", "Storing uninitialized value", "Storing <value>", "Assigning value".
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130412/222b18a5/attachment.html>