Skip to content
This repository was archived by the owner on Sep 1, 2025. It is now read-only.

fix utf8 handling in C code#3

Merged
yujinakayama merged 2 commits into
increments:masterfrom
gfx:fix_utf8_handling
Feb 23, 2017
Merged

fix utf8 handling in C code#3
yujinakayama merged 2 commits into
increments:masterfrom
gfx:fix_utf8_handling

Conversation

@gfx

@gfx gfx commented Jan 30, 2017

Copy link
Copy Markdown

Fix #2, which comes in redcarpet.

Some tests are borrowed from vmg#464

Comment thread greenmat.gemspec
s.require_paths = ["lib"]

s.add_development_dependency "nokogiri", "~> 1.6.0"
s.add_development_dependency "rake", "~> 10.0"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

NOTE: I can't build it with the latest rake, so fixed the version to v10.

@gfx

gfx commented Jan 30, 2017

Copy link
Copy Markdown
Author

CI fails because of activesupport dependency. How can I fix it?

@gfx

gfx commented Feb 14, 2017

Copy link
Copy Markdown
Author

@yujinakayama ping?

@yujinakayama

Copy link
Copy Markdown

Sorry for the late response. Right now I'm busy but will review in this week 🙏

@yujinakayama yujinakayama mentioned this pull request Feb 20, 2017
@yujinakayama

Copy link
Copy Markdown

CI fails because of activesupport dependency. How can I fix it?

The CI issues are fixed in the current master. Could you please rebase against the master? 🙏

Comment thread ext/greenmat/autolink.c

for (link_end = 0; link_end < size; ++link_end) {
uint8_t c = data[link_end];
char c = data[link_end];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I understand this change forces the c to be virtually 7-bit ASCII but it's a bit obscure. Maybe the way in vmg#463 and defining a macro something like is_ascii() (c < 0x7f) would be more explicit.

@gfx gfx Feb 21, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I understand this change forces the c to be virtually 7-bit ASCII

No, it might be a part of UTF-8's middle bytes, and the change of the type doesn't change the bit pattern of c.

Because c is dealt as char, it should be char, not uint8_t (i.e. unsigned char). I believe this code is the best.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All right 👍

@gfx gfx force-pushed the fix_utf8_handling branch from 2718458 to a316a54 Compare February 21, 2017 07:53
@gfx

gfx commented Feb 21, 2017

Copy link
Copy Markdown
Author

Rebased.

@yujinakayama yujinakayama merged commit 6451128 into increments:master Feb 23, 2017
@yujinakayama

Copy link
Copy Markdown

Thanks!

@yujinakayama

Copy link
Copy Markdown

Released as v3.2.2.2.

@gfx

gfx commented Feb 24, 2017

Copy link
Copy Markdown
Author

Thanks!

@gfx gfx deleted the fix_utf8_handling branch February 24, 2017 01:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants