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

readline: use logical assignment operators #39174

Closed
wants to merge 1 commit into from

Conversation

VoltrexKeyva
Copy link
Member

@VoltrexKeyva VoltrexKeyva commented Jun 27, 2021

With the addition of the new logical assignment operators
(||=, ??= and &&=), we could use them here.

With the addition of the new logical assignment operators (`||=`, `??=` and `&&=`), we could use it here.
@github-actions github-actions bot added needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module. labels Jun 27, 2021
@VoltrexKeyva
Copy link
Member Author

On a note, if these changes are alright; could we change this PR to make use of the operators in all the lib modules instead of just this one? Or should we make several different PRs for it?

@aduh95
Copy link
Contributor

aduh95 commented Jun 27, 2021

After what we learned in #38245, I think we should be extra cautious when making this kind of change to existing code. The risk of introducing perf regressions for the sake of a small (debatable) improvement in the readability of code seems like a pretty bad deal IMHO, I would be -1 for anything that touches a hot code path (in the case of readline, I'm -0).

I don't think we should land this kind of change until:

  1. perf concerns have been addressed by V8.
  2. there's a lint rule to enforce it.

could we change this PR to make use of the operators in all the lib modules instead of just this one? Or should we make several different PRs for it?

Keeping the PRs minimal would be the right way to go: it's easier to review, has less risk of getting conflicts with other PRs, and easier to revert.

@VoltrexKeyva
Copy link
Member Author

After what we learned in #38245, I think we should be extra cautious when making this kind of change to existing code. The risk of introducing perf regressions for the sake of a small (debatable) improvement in the readability of code seems like a pretty bad deal IMHO, I would be -1 for anything that touches a hot code path (in the case of readline, I'm -0).

I don't think we should land this kind of change until:

  1. perf concerns have been addressed by V8.
  2. there's a lint rule to enforce it.

could we change this PR to make use of the operators in all the lib modules instead of just this one? Or should we make several different PRs for it?

Keeping the PRs minimal would be the right way to go: it's easier to review, has less risk of getting conflicts with other PRs, and easier to revert.

Understandable, I don't think its worth it to land a small change like this that probably has a chance of either causing a perf regression or something similar until the rules you mentioned such as them being addressed by V8; closing.

@VoltrexKeyva VoltrexKeyva deleted the patch-3 branch June 27, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants