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

refactor: use compile-time cli arg sets. #38748

Merged
merged 2 commits into from
Jun 13, 2023
Merged

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Jun 12, 2023

Description of Change

We're currently building these on the heap with std::set<std::string> but this can be a very small compile-time container instead. (Discussion of base::fixed_flat_map and base::fixed_flat_set here.)

Marking as 'refactor' rather than 'perf' since this isn't called often, but moving from heap to compile-time is good and using this container makes the code more readable.

CC: @dsanders11 who showed interest in string -> StringPiece conversion and @codebytere as the primary for the functions changed here, but code review welcome from anyone

Checklist

Release Notes

Notes: none

We're currently building these on the heap with `std::set<std::string>`
but this can be a very small compile-time container instead.

Marking as 'refactor' rather than 'perf' since this isn't called often,
but moving from heap to compile-time is good and using this container
makes the code more readable.
@ckerr ckerr added semver/patch backwards-compatible bug fixes target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Jun 12, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 12, 2023
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

👍

shell/common/node_bindings.cc Show resolved Hide resolved
shell/common/node_bindings.cc Show resolved Hide resolved
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 13, 2023
@ckerr ckerr merged commit 167c2b2 into main Jun 13, 2023
@ckerr ckerr deleted the refactor/constexpr-cli-flags branch June 13, 2023 19:38
@release-clerk
Copy link

release-clerk bot commented Jun 13, 2023

No Release Notes

@trop
Copy link
Contributor

trop bot commented Jun 13, 2023

I have automatically backported this PR to "25-x-y", please check out #38768

@trop trop bot added in-flight/25-x-y and removed target/25-x-y PR should also be added to the "25-x-y" branch. labels Jun 13, 2023
@trop
Copy link
Contributor

trop bot commented Jun 13, 2023

I have automatically backported this PR to "26-x-y", please check out #38769

@trop
Copy link
Contributor

trop bot commented Jun 13, 2023

I have automatically backported this PR to "24-x-y", please check out #38770

@trop trop bot added in-flight/26-x-y in-flight/24-x-y merged/24-x-y PR was merged to the "24-x-y" branch merged/26-x-y PR was merged to the "26-x-y" branch. and removed target/26-x-y PR should also be added to the "26-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. in-flight/24-x-y in-flight/26-x-y labels Jun 13, 2023
@trop trop bot added merged/25-x-y PR was merged to the "25-x-y" branch. and removed in-flight/25-x-y labels Jun 14, 2023
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
We're currently building these on the heap with `std::set<std::string>`
but this can be a very small compile-time container instead.

Marking as 'refactor' rather than 'perf' since this isn't called often,
but moving from heap to compile-time is good and using this container
makes the code more readable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/24-x-y PR was merged to the "24-x-y" branch merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants