Page MenuHomePhabricator

[AST][RecoveryExpr] Fix the value category for recovery expr.
ClosedPublic

Authored by hokein on Jul 6 2020, 2:22 AM.

Details

Summary

RecoveryExpr was always lvalue, but it is wrong if we use it to model
broken function calls, function call expression has more compliated rules:

  • a call to a function whose return type is an lvalue reference yields an lvalue;
  • a call to a function whose return type is an rvalue reference yields an xvalue;
  • a call to a function whose return type is nonreference type yields a prvalue;

This patch makes the recovery-expr align with the function call if it is
modeled a broken call.

Diff Detail

Event Timeline

hokein created this revision.Jul 6 2020, 2:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2020, 2:22 AM
sammccall added inline comments.Jul 7 2020, 2:59 AM
clang/lib/Sema/SemaOverload.cpp
12945

here we're splitting the type (e.g. int&&) into a type + VK, and passing both to createrecoveryexpr.

Why not do that on recoveryexpr side? e.g. if we request a recoveryexpr of type int&, return an LValue recoveryexpr of type int?

hokein updated this revision to Diff 276325.Jul 8 2020, 12:34 AM
hokein marked an inline comment as done.

address comment.

clang/lib/Sema/SemaOverload.cpp
12945

right, good idea, this is simpler. I was somehow taking CallExpr as a reference when writing this code.

sammccall accepted this revision.Jul 8 2020, 3:30 AM
sammccall added inline comments.
clang/lib/AST/ExprClassification.cpp
147

there's a block of cases with a similar implementation (near OpaqueValueKind), maybe move there

clang/lib/Sema/SemaOverload.cpp
12945

Hmm, this does seem simpler to me but it also seems that a few places deliberately make this mapping between two related concepts explicit.
Maybe we should at least have a comment on createrecoveryexpr that the value category will be inferred from the (reference) type.

clang/test/SemaCXX/recovery-expr-type.cpp
66

I liked the comment explaining the purpose of the test (no crash for wrong value category)

This revision is now accepted and ready to land.Jul 8 2020, 3:30 AM
hokein updated this revision to Diff 276379.Jul 8 2020, 4:51 AM
hokein marked 3 inline comments as done.

address comments.

clang/lib/Sema/SemaOverload.cpp
12945

Hmm, this does seem simpler to me but it also seems that a few places deliberately make this mapping between two related concepts explicit.

yeah, that's true for at least CallExpr. Expr provides a setValueKind method, ideally (if we enable the type propagation), we could call it to set the value category here. I think it is fine to leave it at recoveryexpr side -- our case is really simple.

clang/test/SemaCXX/recovery-expr-type.cpp
66

oops, the comment gets lost during my rebase :(.

This revision was automatically updated to reflect the committed changes.