Skip to content

string-pool: Introduce a strong typedef for StringId #115

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.

Sign up for GitHub

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

Jump to bottom
Draft
johannes-wolf wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

string-pool: Introduce a strong typedef for StringId #115

johannes-wolf wants to merge 5 commits into from

Conversation

@johannes-wolf
Copy link
Collaborator

@johannes-wolf johannes-wolf commented Jul 20, 2025 •
edited
Loading

This allows for a more type-safe API but is a breaking change.

@johannes-wolf johannes-wolf changed the title string-pool: Introduce a strong typedef for Ids string-pool: Introduce a strong typedef for StringId Jul 20, 2025
cursor[bot]

This comment was marked as outdated.

Sign in to view

@github-actions
Copy link

github-actions bot commented Jul 20, 2025 •
edited
Loading

Test Results

 1 files  ±0   1 suites  ±0   2m 55s ⏱️ +2s
53 tests ±0  53 ✅ ±0  0 💤 ±0  0 ❌ ±0 
58 runs  ±0  58 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 1271988. ± Comparison against base commit 1725bbf.

♻️ This comment has been updated with latest results.

@johannes-wolf johannes-wolf force-pushed the string-id-strong-typedef branch from b78df7f to e9fa8c0 Compare July 20, 2025 12:21
cursor[bot]

This comment was marked as outdated.

Sign in to view

@johannes-wolf johannes-wolf force-pushed the string-id-strong-typedef branch from e9fa8c0 to 62fde0b Compare July 20, 2025 12:26
cursor[bot]

This comment was marked as outdated.

Sign in to view

cursor[bot]

This comment was marked as outdated.

Sign in to view

josephbirkner
josephbirkner approved these changes Jul 21, 2025
View reviewed changes
Copy link
Collaborator

@josephbirkner josephbirkner left a comment

Choose a reason for hiding this comment

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

LGTM!

@johannes-wolf
Copy link
Collaborator Author

johannes-wolf commented Jul 24, 2025

@josephbirkner We could also use https://github.com/foonathan/type_safe for the StringHandle.

@josephbirkner
Copy link
Collaborator

josephbirkner commented Jul 31, 2025

That looks like an awesome library in general. I would support going for it.

@johannes-wolf johannes-wolf force-pushed the string-id-strong-typedef branch from c036404 to 1271988 Compare August 4, 2025 12:28
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 4, 2025

Quality Gate Passed Quality Gate passed

Issues
12 New issues
2 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@johannes-wolf
Copy link
Collaborator Author

johannes-wolf commented Aug 4, 2025

I've decided not to use type_safe, as it was very cumbersome to adapt the StingHanlde using strong_typedef to support the features we need (implicit cast from Enum, acting as an index type, etc.).

@johannes-wolf
Copy link
Collaborator Author

johannes-wolf commented Aug 4, 2025

I will merge this after #118 because of API breaks.

@github-actions
Copy link

github-actions bot commented Aug 4, 2025

Package Line Rate Branch Rate Health
include.simfil 67% 48% ➖
include.simfil.model 91% 53% ✔
src 71% 43% ➖
src.model 82% 48% ✔
Summary 74% (2910 / 3940) 44% (2553 / 5791) ➖

Minimum allowed line rate is 50%

@johannes-wolf johannes-wolf marked this pull request as draft October 19, 2025 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cursor cursor[bot] cursor[bot] left review comments

@josephbirkner josephbirkner josephbirkner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@johannes-wolf @josephbirkner