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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
