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

Make getopts count (and thus align/linewrap) in terms of codepoints not ... #8710

Closed

Conversation

pnkfelix
Copy link
Member

...bytes.

(removing previous note about eff-eye-ex'ing #5516 since it actually does not do so, it just gets us half-way.)

@thestinger
Copy link
Contributor

Could you add a FIXME here noting that it still isn't completely right? It really needs to use graphemes + check for double-width characters to be 100% accurate. Of course, we currently lack the Unicode tables required to do that.

@pnkfelix
Copy link
Member Author

There are already FIXME's in the code in question. I'll check if they can use further elaboration.

Also added regression test for the byte vs codepoint issues that the
previous commit fixes.
// here we just need to indent the start of the description
let rowlen = row.len();
let rowlen = str::count_chars(row, 0, row.len());
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason that this isn't row.char_len()?

Other than that, r=me

Copy link
Member Author

Choose a reason for hiding this comment

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

none other than count_chars is the first one my searches through str.rs found. Will fix.

bors added a commit that referenced this pull request Aug 25, 2013
…excrichton

...bytes.

(removing previous note about eff-eye-ex'ing #5516 since it actually does not do so, it just gets us half-way.)
@bors bors closed this Aug 25, 2013
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.

None yet

4 participants