Bug 151917 – Make UCharIterator createIterator(StringView) visible to other classes

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Sukolsak Sakshuwong]

Move the code for creating a UCharIterator from a StringView from CollatorICU.cpp to StringView.h/.cpp so that future patches that will use ucol_strcollIter (including Bug 147604) can use it.

[Sukolsak Sakshuwong]

Created attachment 266722 [details]
Patch

[Darin Adler]

Comment on attachment 266722 [details]
Patch

Why should this be a member function instead of just a function that takes a StringView?

It’s a little bit dangerous to create one of these UCharIterator objects for general use, because the iterator only good as long as the characters pointer to by the StringView are good. The functions we pass to the UCharIterator don’t do the CHECK_STRINGVIEW_LIFETIME checking.

[Sukolsak Sakshuwong]

(In reply to comment #2)
> Comment on attachment 266722 [details]
> Patch
> 
> Why should this be a member function instead of just a function that takes a
> StringView?
> 
> It’s a little bit dangerous to create one of these UCharIterator objects for
> general use, because the iterator only good as long as the characters
> pointer to by the StringView are good. The functions we pass to the
> UCharIterator don’t do the CHECK_STRINGVIEW_LIFETIME checking.

Could you please advise me on how to go about this? Should I just change it to a function that takes a StringView while keeping it in StringView.h, or should I not do this in general? If the latter, is there a way to avoid duplicate code?

[Darin Adler]

(In reply to comment #3)
> Could you please advise me on how to go about this? Should I just change it
> to a function that takes a StringView while keeping it in StringView.h, or
> should I not do this in general? If the latter, is there a way to avoid
> duplicate code?

As a first step, I’d suggest changing it to a function that takes a StringView and putting the function in Collator.h. Later we can move it somewhere else if we want to use UCharIterator for things other than collation.

The Collator abstraction is not super-important since we are OK with WebKit using ICU directly. It was created by someone who was trying to change WebKit to work with multiple international libraries, but that’s not really a goal for the project now.

Anyway, I think having UCharIterator createUCharIterator(StringView) in Collator.h would be a fine way to share it with other code.

[Sukolsak Sakshuwong]

Created attachment 267286 [details]
Patch

[Darin Adler]

Comment on attachment 267286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267286&action=review

> Source/WTF/wtf/unicode/Collator.h:71
> +UCharIterator createIterator(StringView);

Might later find that we need a more specific name like createUCharIterator but lets start with this.

[WebKit Commit Bot]

Comment on attachment 267286 [details]
Patch

Clearing flags on attachment: 267286

Committed r194041: <http://trac.webkit.org/changeset/194041>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.