Skip to content

Implement ubrk.h #170

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

Merged
merged 9 commits into from
Sep 17, 2020
Merged

Implement ubrk.h #170

merged 9 commits into from
Sep 17, 2020

Conversation

bdragon
Copy link
Contributor

@bdragon bdragon commented Sep 14, 2020

Adds bindings for ubrk.h

Closes #166

API

ICU4C API Rust API
ubrk_close impl Drop
ubrk_next impl Iterator
ubrk_countAvailable count_available_locales
ubrk_getAvailable get_available_locale_at
ubrk_open try_new, try_new_ustring
ubrk_openRules try_new_rules, try_new_rules_ustring
ubrk_openBinaryRules try_new_binary_rules, try_new_binary_rules_ustring
ubrk_getBinaryRules get_binary_rules
ubrk_safeClone safe_clone
ubrk_setText set_text, set_text_ustring
ubrk_setUText Not implemented.1
ubrk_refreshUText Not implemented.1
ubrk_previous previous
ubrk_current current
ubrk_preceding preceding
ubrk_following following
ubrk_first first
ubrk_last last_boundary2
ubrk_isBoundary is_boundary
ubrk_getLocaleByType get_locale_by_type
ubrk_getRuleStatus get_rule_status
ubrk_getRuleStatusVec get_rule_status_vec

1 In light of #6 and the existing UText implementation, it made sense to leave these out for now.

2 The Rust Iterator trait already provides a last() method, so I had to name this differently in order to avoid a conflict.

@google-cla google-cla bot added the cla: yes CLA signed label Sep 14, 2020
@bdragon
Copy link
Contributor Author

bdragon commented Sep 14, 2020

Minor thing: I noticed that when the simple_drop_impl! macro is used, make cov does not treat the referenced ICU *_close API as implemented in the coverage report, so I added an Implements ... doc comment above the macro. This caused make cov to consider the ICU API implemented, at the expense of a warning:

95 | /// Implements `ubrk_close`.
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ rustdoc does not generate documentation for macro invocations

@filmil
Copy link
Member

filmil commented Sep 14, 2020

Minor thing: I noticed that when the simple_drop_impl! macro is used, make cov does not treat the referenced ICU *_close API as implemented in the coverage report, so I added an Implements ... doc comment above the macro. This caused make cov to consider the ICU API implemented, at the expense of a warning:

95 | /// Implements `ubrk_close`.
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ rustdoc does not generate documentation for macro invocations

You will note that elsewhere I use // Implements ... instead of ... where the macro is invoked to work around this issue.
Since the macro expansion itself contains the three-slashed version of the same text, the end result comes out OK in both rustdoc and compilation - without warnings.

@bdragon
Copy link
Contributor Author

bdragon commented Sep 15, 2020

Thanks. Updated in d1876d5.

@filmil
Copy link
Member

filmil commented Sep 15, 2020

I'm now looking at this PR.

@bdragon
Copy link
Contributor Author

bdragon commented Sep 15, 2020

I found a couple of broken links in the doc comments. Fixed in 1932074.

@bdragon
Copy link
Contributor Author

bdragon commented Sep 16, 2020

@filmil I appreciate the review. I believe I have addressed your feedback if you don't mind taking another look.

@filmil
Copy link
Member

filmil commented Sep 16, 2020

Thank you for making the changes, I'll take a look as soon as able, but at the moment it seems will need to happen tomorrow.

@filmil
Copy link
Member

filmil commented Sep 17, 2020

Re-reviewing now.

@filmil
Copy link
Member

filmil commented Sep 17, 2020

Thank you for your patience for the review. This is now approved.

@filmil filmil merged commit f5d9b55 into google:master Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ubrk.h
2 participants