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

std.utf: Add isValidCodepoint. #8043

Merged
merged 1 commit into from
Jul 5, 2021
Merged

Conversation

berni44
Copy link
Contributor

@berni44 berni44 commented May 4, 2021

For improving formatting characters with std.format I need this function (e.g. fixing issue 21815, but can also be used for issue 6125). I could add it as a private function to std.format, but I think, it fits better in here and might be useful for users too. Especially, because the dchar version already exists here as isValidDchar, while isValidWchar and isValidChar are missing (and it's not D like to use these functions instead of a single template).

I did not manage to find out, why isValidDchar was designed like this. It was added in 2007 to git, but probably existed prior to that somewhere else. Eventually, isValidCharacter might replace isValidDchar. (Should I do so immediately?)

cc @atilaneves

@dlang-bot
Copy link
Contributor

dlang-bot commented May 4, 2021

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Your 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 locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8043"

@dukc
Copy link
Contributor

dukc commented May 5, 2021

What's the problem with isValidDChar? char and wchar convert implicitly to dchar so the old function can be used with all character types.

I suggest a different name. This function is named as if it was testing the validity of a code unit. isCodePoint would be much better.

@aG0aep6G
Copy link
Contributor

aG0aep6G commented May 5, 2021

What's the problem with isValidDChar? char and wchar convert implicitly to dchar so the old function can be used with all character types.

dchar(0x80) represents the code point U+0080. char(0x80) does not represent a code point. Having char and wchar implicitly convert to dchar is a mistake at the language level. isValidDchar does not give sensible results for all character types.

@dukc
Copy link
Contributor

dukc commented May 5, 2021

dchar(0x80) represents the code point U+0080. char(0x80) does not represent a code point. Having char and wchar implicitly convert to dchar is a mistake at the language level.

Or at least they should convert to dchar.init when the source has only a partial code point. But anyway, this explains why we might want a new function.

std/utf.d Show resolved Hide resolved
@berni44 berni44 changed the title std.utf: Add isValidCharacter. std.utf: Add isValidCodepoint. May 10, 2021
@berni44 berni44 closed this May 20, 2021
@dukc
Copy link
Contributor

dukc commented May 20, 2021

@berni44 You got tired of waiting for review?

@atilaneves is there anything anymore blocking merging this right away?

@thewilsonator thewilsonator reopened this Jun 22, 2021
@thewilsonator thewilsonator added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jun 22, 2021
@thewilsonator
Copy link
Contributor

Sorry for the late review. Looks fine.

@RazvanN7 RazvanN7 added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Jul 5, 2021
@dlang-bot dlang-bot merged commit b3e9f42 into dlang:master Jul 5, 2021
@MoonlightSentinel
Copy link
Contributor

The changelog was not updated to the new name!

https://issues.dlang.org/show_bug.cgi?id=22503

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

Successfully merging this pull request may close these issues.

8 participants