This is an archive of the discontinued LLVM Phabricator instance.

PR15677 - Crash in template diffing.
ClosedPublic

Authored by nikola on Jun 19 2014, 7:22 PM.

Details

Reviewers
rtrieu
Summary

In addition to having and integral type the expression needs to be evaluatable.

I'm not too happy with how GetInt function looks... one idea would be to return a bool and take APInt as an output parameter, in that case the function would look something like this:

bool GetInt(const TSTiterator &Iter, Expr *ArgExpr, llvm::APInt &Int) {
  // Default, value-depenedent expressions require fetching
  // from the desugared TemplateArgument
  if (Iter.isEnd() && ArgExpr->isValueDependent()) {
    switch (Iter.getDesugar().getKind()) {
    case TemplateArgument::Integral:
      Int = Iter.getDesugar().getAsIntegral();
    case TemplateArgument::Expression:
      ArgExpr = Iter.getDesugar().getAsExpr();
      Int = ArgExpr->EvaluateKnownConstInt(Context);
    default:
      llvm_unreachable("Unexpected template argument kind");
    }
    return true;
  } else if (ArgExpr->isEvaluatable(Context)) {
    Int = ArgExpr->EvaluateKnownConstInt(Context);
    return true;
  }

  return false;
}

Diff Detail

Event Timeline

nikola updated this revision to Diff 10676.Jun 19 2014, 7:22 PM
nikola retitled this revision from to PR15677 - Crash in template diffing..
nikola updated this object.
nikola edited the test plan for this revision. (Show Details)
nikola added a reviewer: rtrieu.
nikola updated this object.
nikola added a subscriber: Unknown Object (MLST).
rtrieu edited edge metadata.Jun 20 2014, 7:37 PM

That seems rather special. I'm not entirely convinced that all the edge are taken care of here. I will be able to take a closer look at this patch next week.

Besides the above, PrintAPSInt needs to updated since a new node format is being introduced. On the else branch where "(no argument)" is printed, this should be changed to print the expression if E is valid, and print "(no argument)" if it is not.

lib/AST/ASTDiagnostic.cpp
970

This needs another case here, (HasFromInt || HasToInt). Use same SetNode and SetKind as above, but use SetSame(false).

1122

Go with version you put in the summary: returns bool and takes APInt as an reference argument. Update the comment here.

test/Misc/diag-template-diffing.cpp
1100

Add another test case, just like E, but use 21 + 21 and test that "21 + 21 aka 42" is printed.

nikola updated this revision to Diff 10990.Jun 30 2014, 7:28 PM
nikola edited edge metadata.

Good catch Richard, I had the feeling that I've missed something... This patch should address issues you raised.

P.S. I have commit access.

rtrieu added inline comments.Jun 30 2014, 7:44 PM
lib/AST/ASTDiagnostic.cpp
1131–1137

Why did you change from a switch statement to an if chain here?

nikola added inline comments.Jun 30 2014, 7:47 PM
lib/AST/ASTDiagnostic.cpp
1131–1137

The entire function was coded in Phabricator and was missing breaks. After adding those it seem to be a fair bit longer than needed. Do you want me to change it?

rtrieu accepted this revision.Jun 30 2014, 8:17 PM
rtrieu edited edge metadata.

LGTM

Address the one comment and you are ready to submit.

lib/AST/ASTDiagnostic.cpp
1131–1137

Yes, please change it back. Also, use "return true;" instead of breaks and remove the "return true;" below. This mirrors the old codeflow better.

This revision is now accepted and ready to land.Jun 30 2014, 8:17 PM
nikola closed this revision.Jun 30 2014, 9:26 PM

r212090. Thanks for the review.