GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: use `enum class` instead of `enum` in node_i18n by deokjinkim · Pull Request #45646 · nodejs/node

Conversation

deokjinkim commented Nov 27, 2022

Copy link

Copy Markdown

Contributor

"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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with eyes emoji

All reactions

👀 1 reaction

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 Node.js internationalization support.

needs-ci PRs that need a full CI run. labels

Nov 27, 2022

deokjinkim force-pushed the 221127_use_enum_class branch 2 times, most recently from 2ea515e to eb666b2 Compare

November 27, 2022 12:13

tniessen reviewed

Nov 27, 2022

View reviewed changes

Comment thread

src/node_i18n.h Outdated

tniessen Nov 27, 2022

Copy link

Copy Markdown

Member

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell, anonrig, and RaisinTen reacted with thumbs up emoji

All reactions

👍 3 reactions

deokjinkim Nov 28, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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 think good suggestion from a clean code point of view. Removed IDNA_ prefix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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 force-pushed the 221127_use_enum_class branch from eb666b2 to bc0541f Compare

November 28, 2022 01:49

deokjinkim commented Nov 28, 2022

Copy link

Copy Markdown

Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          remove IDNA_ prefix from enum idna_mode

5d4cb6f

deokjinkim force-pushed the 221127_use_enum_class branch from bc0541f to 5d4cb6f Compare

November 28, 2022 04:14

targos commented Nov 28, 2022

Copy link

Copy Markdown

Member

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]

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Add k prefix to each element of enum

b197bf1

This change is needed to fix error of window build.

RaisinTen reviewed

Nov 28, 2022

View reviewed changes

Comment thread

src/node_i18n.h Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Apply k followed by the name in PascalCase

574af27

RaisinTen approved these changes

Nov 28, 2022

View reviewed changes

deokjinkim commented Nov 28, 2022

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tniessen approved these changes

Nov 28, 2022

View reviewed changes

tniessen added author ready

PRs with CI started, the required approvals, and no outstanding review comments.

request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels

Nov 28, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Nov 28, 2022

This comment was marked as outdated.

github-actions Bot mentioned this pull request

Nov 29, 2022

CI Reliability 2022-11-29 nodejs/reliability#443

Open

16 tasks

JungMinu approved these changes

Nov 29, 2022

View reviewed changes

nodejs-github-bot commented Nov 29, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot mentioned this pull request

Nov 30, 2022

CI Reliability 2022-11-30 nodejs/reliability#444

Open

15 tasks

tniessen added commit-queue

PRs queued for automated landing through the Commit Queue.

commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels

Nov 30, 2022

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Nov 30, 2022

nodejs-github-bot merged commit 1750a94 into nodejs:main

Nov 30, 2022

nodejs-github-bot commented Nov 30, 2022

Copy link

Copy Markdown

Collaborator

Landed in 1750a94

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Dec 1, 2022

CI Reliability 2022-12-01 nodejs/reliability#445

Open

CI Reliability 2022-12-02 nodejs/reliability#446

Open

CI Reliability 2022-12-03 nodejs/reliability#447

Open

CI Reliability 2022-12-04 nodejs/reliability#448

Open

CI Reliability 2022-12-05 nodejs/reliability#449

Open

CI Reliability 2022-12-07 nodejs/reliability#450

Open

targos pushed a commit that referenced this pull request

Dec 12, 2022


          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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>

targos mentioned this pull request

Dec 12, 2022

v19.3.0 release proposal #45831

Merged

danielleadams mentioned this pull request

Dec 30, 2022

v18.13.0 release proposal #46025

Merged

danielleadams pushed a commit that referenced this pull request

Dec 30, 2022


          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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>

danielleadams pushed a commit that referenced this pull request

Jan 3, 2023


          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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>

danielleadams pushed a commit that referenced this pull request

Jan 5, 2023


          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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>

Reviewers

tniessen

tniessen approved these changes

JungMinu

JungMinu approved these changes

RaisinTen

RaisinTen approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

c++

Issues and PRs that require attention from people who are familiar with C++.

commit-queue-squash

PRs the Commit Queue should land as one squashed commit.

i18n-api

Issues and PRs related to Node.js internationalization support.

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

deokjinkim commented Nov 27, 2022

Uh oh!

tniessen Nov 27, 2022

Choose a reason for hiding this comment

Uh oh!

deokjinkim Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deokjinkim commented Nov 28, 2022

Uh oh!

targos commented Nov 28, 2022

Uh oh!

Uh oh!

deokjinkim commented Nov 28, 2022

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Nov 29, 2022

Uh oh!

nodejs-github-bot commented Nov 30, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

deokjinkim Nov 28, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.