Uh oh!
There was an error while loading. Please reload this page.
Re-implement ExprIdentifierVisitor::desc_expr to use Expr::Display - #3339
Conversation
andygrove
commented
Sep 2, 2022
@waynexia Does this look reasonable? |
bd5ae23 to
81cc956Compare
waynexia
left a comment
There was a problem hiding this comment.
Thanks for bringing this up. This Display way looks more pretty, I like it!
ursabot
commented
Sep 3, 2022
Benchmark runs are scheduled for baseline = e26452a and contender = 786c319. 786c319 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #3330
Rationale for this change
Expralready implements theDisplaytrait so it doesn't seem necessary forExprIdentifierVisitorto have an alternate way to build descriptions ofExpr.The
ExprIdentifierVisitor::desc_exprgenerated names are hard to interpret as well, IMO, so better to use the full description.For example:
What changes are included in this PR?
ExprIdentifierVisitornow just delegates toimpl Display for ExprAre there any user-facing changes?
Plans will look different