-
-
Notifications
You must be signed in to change notification settings - Fork 746
Added a new popGrapheme function to std.uni #9053
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
Conversation
Thanks for your pull request and interest in making D better, @dukc! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#9053" |
ping @atilaneves |
The new function is a cross between the existing $(REF graphemeStride, std, | ||
uni) and $(REF decodeGrapheme, std, uni) functions. The new function both | ||
supports `@safe pure nothrow @nogc` like `graphemeStride` does as long as you | ||
don't rely on autodecoding (side node: `@nogc` support for `graphemeStride` |
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.
This side note sounds like it should be its own PR.
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.
It is. I felt an added @nogc
compability to an existing funtion alone wasn't worth a changelog entry so didn't add one in that PR. But since I'm writing here that graphemeStride
is @nogc
anyway, which until very recently wasn't the case, I feel it's useful to mention it. As in: "this note applies only with the latest compiler/Phobos".
I don't know how useful this is so I would like someone that knows/cares about unicode more than me opines. Not sure who. @rikkimax ? |
It allows you to get the grapheme length extracted, so you can slice and dice your string without allocation. Kinda like It is one of those things that if a unicode library is being used you'll end up having a hundred variations of. I see nothing wrong with wanting functions like this. |
Maybe @John-Colvin ? I think he was supposed to take charge, at one point at least, of my Unicode-related project at Symmetry. |
Or alternatively @DmitryOlshansky . |
So you're saying we should merge it? |
I have no argument against merging. It is sound, it's a variation of something that exists and is worth having different variants for. The reason I'm avoiding the phrase "merge it" is because of PhobosV3 vs V2. I don't know how you and @LightBender want to handle it. |
I'm good with merging this. Need to get it passing the build though. |
Fails at some of the platforms but passes on others, and I'm not doing anything platform-specific. So probably spurious. I'll attempt restarting the tests. EDIT: Done now. For transparency, I'll note that I made a small change while there: added |
module doc index while there.
It seems that the Mac machines are broken. Most likely this PR is not at fault. |
Well Mac machines don't apparently have the "required" labes so probably should just ignore them. @atilaneves good to go now? |
I'm going to merge this as there has been no dissent to date. |
Since it's Hackathon today, even if I'm not there I decided to do something in similar spirit.
@atilaneves new symbol canditate for phobos, meaning it needs your approval!