Skip to content

Propose new ClassName API#58

Merged
staktrace merged 4 commits into
staktrace:mainfrom
xitep:main
Jun 2, 2025
Merged

Propose new ClassName API#58
staktrace merged 4 commits into
staktrace:mainfrom
xitep:main

Conversation

@xitep

@xitep xitep commented May 30, 2025

Copy link
Copy Markdown
Contributor

please consider this just a possible proposal with regards to #52; more work would be necessary to cover also other places as mentioned by @wuwbobo2021 here; i'm willing to carry on with it, if you're fine with the simplistic api.

@staktrace

staktrace commented Jun 1, 2025

Copy link
Copy Markdown
Owner

Thanks for the PR!

On the whole this looks good but there were a bunch of changes in here that seemed unrelated to the main goal. For example the implementation of parse_unqualified_segments was rewritten in a style that (to me) was less readable. The reason for the changes around the pub mod/pub use stuff in lib.rs was also not clear to me.

I'm going to revert some of the changes I believe are spurious, and I'm also making a change to the try_from implementation so that it reuses the parse_unqualified_segments function instead of effectively reimplementing the parsing validation.

@staktrace

Copy link
Copy Markdown
Owner

Please take a look at the changes I made. If it looks good to you I'm happy to merge this.

@xitep

xitep commented Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

thanks for the update @staktrace; i'm fully fine with your changes.

  • i'm sorry for the complexity in parse_unqualified_segments, i seem to got carried away and tried to take out the comparison against the index out of the hot loop, but now i believe it didn't bring much.
  • the changes around pub/mod was to move ClassName to the top-level of the crate (since I though it would be shared more broadly through cafebabe.) but it's fine with me as it is now.
  • the originally "duplicate" implementation in try_from was deliberate to make the code faster when wrapping the classnames. but i'm ok with reusing parse_unqualified_segments .. it's an implementation detail and does not affect the overall api.

many thanks for having had a look and your hands on it. feel free to merge.

@staktrace staktrace merged commit 109bf44 into staktrace:main Jun 2, 2025
5 checks passed
@staktrace

Copy link
Copy Markdown
Owner

Great, thank you!

@xitep

xitep commented Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

btw: i was curious about the performance difference in the two implementations; your version of parse_unqualified_segments is slightly faster. so pulling the index comparison out of the hot loop (as i initially proposed) obviously has no benefit. on the other hand, the inlined try_from implementation (as i had it originally) is about 15-20% faster than the now merged version; here's a basic benchmark: https://github.com/xitep/cafebabe/pull/1

@staktrace

Copy link
Copy Markdown
Owner

Finally got around to looking at this. I improved the perf a bit in #60 but you're right that your original implementation is still faster. Still, unless it's shown to be the bottleneck in a real-world use case I'd rather not over-optimize at the expense of code maintainability/readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants