Skip to content

src: use enum class instead of enum in node_i18n#45646

Merged
nodejs-github-bot merged 4 commits intonodejs:mainfrom
deokjinkim:221127_use_enum_class
Nov 30, 2022
Merged

src: use enum class instead of enum in node_i18n#45646
nodejs-github-bot merged 4 commits intonodejs:mainfrom
deokjinkim:221127_use_enum_class

Conversation

@deokjinkim
Copy link
Copy Markdown
Contributor

@deokjinkim deokjinkim commented Nov 27, 2022

"enum class" has more advantages than "enum" because it's strongly typed and scoped.

Refs: https://isocpp.org/wiki/faq/cpp11-language-types#enum-class

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. needs-ci PRs that need a full CI run. labels Nov 27, 2022
@deokjinkim deokjinkim force-pushed the 221127_use_enum_class branch 2 times, most recently from 2ea515e to eb666b2 Compare November 27, 2022 12:13
tniessen
tniessen reviewed Nov 27, 2022
View reviewed changes
Comment thread src/node_i18n.h
void SetDefaultTimeZone(const char* tzid);

enum idna_mode {
enum class idna_mode {
Copy link
Copy Markdown
Member

@tniessen tniessen Nov 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing the IDNA_ prefix from the members of idna_mode since the scope is changing. For example, idna_mode::IDNA_STRICT should just be idna_mode::STRICT.

Copy link
Copy Markdown
Contributor Author

@deokjinkim deokjinkim Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think good suggestion from a clean code point of view. Removed IDNA_ prefix.

@deokjinkim
src: use enum class instead of enum in node_i18n
ff3020e 
"enum class" has more advantages than "enum" because
it's strongly typed and scoped.

Refs: https://isocpp.org/wiki/faq/cpp11-language-types#enum-class
@deokjinkim deokjinkim force-pushed the 221127_use_enum_class branch from eb666b2 to bc0541f Compare November 28, 2022 01:49
@deokjinkim
Copy link
Copy Markdown
Contributor Author

deokjinkim commented Nov 28, 2022

@tniessen BTW, I have one question about test. For now, Build Windows and Coverage Windows are failed, but I don't know why these tests are failed. It's very helpful if you let me know how I can resolve this case.

@deokjinkim deokjinkim force-pushed the 221127_use_enum_class branch from bc0541f to 5d4cb6f Compare November 28, 2022 04:14
@targos
Copy link
Copy Markdown
Member

targos commented Nov 28, 2022

Windows errors:

https://github.com/nodejs/node/actions/runs/3561646551/jobs/5983757408#step:6:2624

D:\a\node\node\src\node_i18n.h(53,3): error C2143: syntax error: missing '}' before 'constant' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(53,3): error C2059: syntax error: 'constant' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(54,1): error C2143: syntax error: missing ';' before '}' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(61,17): error C2061: syntax error: identifier 'idna_mode' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(139,1): error C2059: syntax error: '}' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(139,1): error C2143: syntax error: missing ';' before '}' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\inspector_socket.h(12,16): error C2143: syntax error: missing ';' before '{' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\inspector_socket.h(12,16): error C2447: '{': missing function header (old-style formal list?) [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\inspector_socket_server.h(85,25): error C2061: syntax error: identifier 'InspectorSocket' [D:\a\node\node\libnode.vcxproj]

@deokjinkim
Add k prefix to each element of enum
b197bf1 
This change is needed to fix error of window build.
RaisinTen
RaisinTen reviewed Nov 28, 2022
View reviewed changes
Comment thread src/node_i18n.h Outdated
@deokjinkim
Copy link
Copy Markdown
Contributor Author

deokjinkim commented Nov 28, 2022

Windows errors:

https://github.com/nodejs/node/actions/runs/3561646551/jobs/5983757408#step:6:2624

D:\a\node\node\src\node_i18n.h(53,3): error C2143: syntax error: missing '}' before 'constant' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(53,3): error C2059: syntax error: 'constant' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(54,1): error C2143: syntax error: missing ';' before '}' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(61,17): error C2061: syntax error: identifier 'idna_mode' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(139,1): error C2059: syntax error: '}' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_i18n.h(139,1): error C2143: syntax error: missing ';' before '}' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\inspector_socket.h(12,16): error C2143: syntax error: missing ';' before '{' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\inspector_socket.h(12,16): error C2447: '{': missing function header (old-style formal list?) [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\inspector_socket_server.h(85,25): error C2061: syntax error: identifier 'InspectorSocket' [D:\a\node\node\libnode.vcxproj]

Thanks to your sharing, I could find solution to avoid error of window build.

@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 28, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions Bot mentioned this pull request Nov 29, 2022
Open
16 tasks
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Nov 29, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/48236/

@github-actions github-actions Bot mentioned this pull request Nov 30, 2022
Open
15 tasks
@tniessen tniessen added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Nov 30, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 30, 2022
@nodejs-github-bot nodejs-github-bot merged commit 1750a94 into nodejs:main Nov 30, 2022
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Nov 30, 2022

Landed in 1750a94

ErickWendel pushed a commit to ErickWendel/node that referenced this pull request Nov 30, 2022
@deokjinkim @ErickWendel
src: use enum class instead of enum in node_i18n
fb3ed02 
"enum class" has more advantages than "enum" because
it's strongly typed and scoped.

Refs: https://isocpp.org/wiki/faq/cpp11-language-types#enum-class
PR-URL: nodejs#45646
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
This was referenced Dec 1, 2022
Open
Open
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Dec 12, 2022
@deokjinkim @targos
src: use enum class instead of enum in node_i18n
c9cba2e 
"enum class" has more advantages than "enum" because
it's strongly typed and scoped.

Refs: https://isocpp.org/wiki/faq/cpp11-language-types#enum-class
PR-URL: #45646
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@targos targos mentioned this pull request Dec 12, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@deokjinkim @danielleadams
src: use enum class instead of enum in node_i18n
b9c2151 
"enum class" has more advantages than "enum" because
it's strongly typed and scoped.

Refs: https://isocpp.org/wiki/faq/cpp11-language-types#enum-class
PR-URL: #45646
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@deokjinkim @danielleadams
src: use enum class instead of enum in node_i18n
c1ed3cf 
"enum class" has more advantages than "enum" because
it's strongly typed and scoped.

Refs: https://isocpp.org/wiki/faq/cpp11-language-types#enum-class
PR-URL: #45646
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@deokjinkim @danielleadams
src: use enum class instead of enum in node_i18n
ed0a867 
"enum class" has more advantages than "enum" because
it's strongly typed and scoped.

Refs: https://isocpp.org/wiki/faq/cpp11-language-types#enum-class
PR-URL: #45646
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@deokjinkim @danielleadams
src: use enum class instead of enum in node_i18n
936a5c0 
"enum class" has more advantages than "enum" because
it's strongly typed and scoped.

Refs: https://isocpp.org/wiki/faq/cpp11-language-types#enum-class
PR-URL: #45646
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
@deokjinkim @danielleadams
src: use enum class instead of enum in node_i18n
e1c6ec9 
"enum class" has more advantages than "enum" because
it's strongly typed and scoped.

Refs: https://isocpp.org/wiki/faq/cpp11-language-types#enum-class
PR-URL: #45646
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen approved these changes

@JungMinu JungMinu JungMinu approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. i18n-api Issues and PRs related to the i18n implementation. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@deokjinkim @targos @nodejs-github-bot @tniessen @JungMinu @RaisinTen
⚡