-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
src: add ASCII table constants #18743
Conversation
Since we are refactoring ASCII character values into another separate file, as in the referred PR. Why not create the entire ASCII character to values mapping for future reference and completeness. Refs: #18654
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know there’s been some discussion around whether we should make such a change at all, but I’m not really opposed to it at least. The identifiers should imo be a bit easily understandable, though.
One question straight ahead: Are you willing to backport this to all active release branches (v9.x, v8.x, v6.x) yourself manually? There’s a decent chance that will be necessary.
UPPER_Y: 89, | ||
UPPER_Z: 90, | ||
LBRACK: 91, | ||
BSLASH: 92, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you spell out BACKSLASH
, NUMBER_SIGN
, LEFT_BRACKET
etc?
Ideally we’d use something pretty close to the official Unicode identifiers, although those might be a bit too verbose sometimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally also preferred the former more verbose naming with LOWERCASE
and UPPERCASE
.
I'll definitely be willing to learn how to backport this to all necessary releases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will need to think about this a bit but a bit of feedback to start: there was a specific reason that the previous PR went with CHAR_XYZ convention and it's because using the ASCII namespace will be a good bit slower. That's an extra lookup each time as compared to destructuring into constants.
I'm not opposed to this PR. But if we really need to do this, I think it is better to move these ASCII constants to a separate file (and a name space), like |
CHAR_COLON, | ||
CHAR_QUESTION_MARK, | ||
} = require('internal/constants'); | ||
const { ASCII } = require('internal/constants'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This results in a potential extra lookup and unnecessary churn. It can be simplified by changing the original line 35 to } = require('internal/constants').ASCII;
Due to the name change of the constants it would still be necessary to change the entries here but the extra lookup would be gone.
UPPER_Y: 89, | ||
UPPER_Z: 90, | ||
LBRACK: 91, | ||
BSLASH: 92, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally also preferred the former more verbose naming with LOWERCASE
and UPPERCASE
.
Right now we have multiple entries that still have constants to be migrated. What I am certain about is that we do not use the whole ASCII table but only a fraction of it. Therefore I would really not like to add the whole table. Instead, I would like to only add the entries when we need them. Adding the namespace is somewhat the same as we do not have anything besides ASCII right now. As soon as we port something that is not ASCII, I would be for adding namespaces. |
I completely agree with @BridgeAR |
Alright. In that case, would it be better if I close this PR and let whoever that is refactoring the constants to add entries to the table? |
That would be my preference. I do not know what others think about that. |
as long as this file only exists for ascii stuff, maybe |
I am fine with renaming the constants file to |
This has a lot of conflicts right now and there are still questions about this being the right way to progress or not. @nodejs/collaborators PTAL |
Sidenote: Subsystem probably shouldn't be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 for several reasons:
-
I don't think we should have the whole ASCII table unless it's needed.
-
Adding a namespace doesn't seem very useful. We might have < 10 chars long-term that are not ASCII. It's unclear what other constants might get added but we should address that when it happens not optimize prematurely.
-
The naming convention here is worse than what exists currently for several reasons: it's less descriptive, some names are unnecessarily shortened and it doesn't lend itself well to being destructured into standalone constants because "ENQ", "US" or "SLASH" are not very clear variable names.
-
The changes to
path.js
are nothing but churn of variable names.
You can't +1 a review in GitHub so I'll write that I mostly agree with @apapirovski |
I am closing this due to the mentioned reasons. @jiaherrt thanks a lot for your contribution anyway! It is much appreciated. |
Since we are refactoring ASCII character values into another
separate file, as in the referred PR. Why not create the entire
ASCII character to values mapping for future reference and
completeness.
Refs: #18654
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)