Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

as_label() and expr_text() should use expr_deparse() #357

Open
hadley opened this issue Jan 16, 2018 · 1 comment
Open

as_label() and expr_text() should use expr_deparse() #357

hadley opened this issue Jan 16, 2018 · 1 comment

Comments

@hadley
Copy link
Member

hadley commented Jan 16, 2018

Instead of deparse

lionel- added a commit to lionel-/rlang that referenced this issue Jan 21, 2018
lionel- added a commit to lionel-/rlang that referenced this issue Jan 21, 2018
lionel- added a commit to lionel-/rlang that referenced this issue Jan 21, 2018
@lionel-
Copy link
Member

lionel- commented Jan 21, 2018

It is probably too early to switch to expr_deparse() right away. I've just fixed two bugs while trying to switch: fa47703 and 6a2b6bb. Since expr_label() is now used in testthat we should test-drive the deparsing algorithm for a while.

Also there's the performance issue. expr_deparse() is very slow because:

  • it's a recursive function written in R;
  • it reallocates a new string each time each new characters are added to the last line and a whole new character vector of lines each time a new line is added (which is likely costly even if it's only a vector of pointers to interned strings).

Once it's feature-complete it should be rewritten in C with an extensible buffer, perhaps with a non-recursive algorithm.

@lionel- lionel- added the print label Jan 26, 2018
@lionel- lionel- changed the title expr_text should use expr_deparse as_label() and expr_text() should use expr_deparse() Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants