[PATCH] D19084: [scan-build] fix warnings emitted on Clang AST code base

[Richard Trieu via cfe-commits]

rtrieu added inline comments.

================
Comment at: lib/AST/ASTDiagnostic.cpp:1686
@@ -1685,3 +1685,3 @@
 
-    if (Same) {
+    if (Same && FromTD) {
       OS << "template " << FromTD->getNameAsString();
----------------
apelete wrote:
> rtrieu wrote:
> > dblaikie wrote:
> > > Should this be a condition, or just an assertion?
> > This should be an assertion.  Same == true implies FromTD and ToTD are not null.
> What do you mean by "This should be an assertion" ?
> There's already an assertion on FromTD and ToTD values at the beginning of PrintTemplateTemplate() so duplicating it here didn't make sense to me: should I replace the if() with an assertion anyway ?
Given the choice between changing the condition and adding an assertion, the assertion is the correct choice.  If Same is true, the first branch must be taken or the wrong message will be printed.

The assertion in this function is (FromTD || ToTD) which allows for FromTD to be null.  That means only the case of FromTD is null and TD is null is excluded.  This allows the case for for FromTD to be null and ToTD to be non-null, which is case the static analysis is warning about.  However, template diffing only sets Same to true when both FromTD and ToTD are non-null, but this isn't checked or enforced anywhere.  

To be explicit, this is the kind of assertion and the location it should be present:
```
if (Same) {
  assert(FromTD && ToTD &&
         "Same implies both template arguments are non-null.");
```


http://reviews.llvm.org/D19084