Bug 151917

Summary: Make UCharIterator createIterator(StringView) visible to other classes
Product: WebKit Reporter: Sukolsak Sakshuwong <sukolsak>
Component: Web Template FrameworkAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, darin, sukolsak
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Sukolsak Sakshuwong 2015-12-05 15:02:05 PST
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.
Comment 1 Sukolsak Sakshuwong 2015-12-05 15:06:12 PST
Created attachment 266722 [details]
Patch
Comment 2 Darin Adler 2015-12-06 18:38:17 PST
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.
Comment 3 Sukolsak Sakshuwong 2015-12-13 12:50:45 PST
(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?
Comment 4 Darin Adler 2015-12-13 16:31:55 PST
(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.
Comment 5 Sukolsak Sakshuwong 2015-12-14 01:52:45 PST
Created attachment 267286 [details]
Patch
Comment 6 Darin Adler 2015-12-14 08:20:06 PST
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.
Comment 7 WebKit Commit Bot 2015-12-14 11:42:10 PST
Comment on attachment 267286 [details]
Patch

Clearing flags on attachment: 267286

Committed r194041: <http://trac.webkit.org/changeset/194041>
Comment 8 WebKit Commit Bot 2015-12-14 11:42:15 PST
All reviewed patches have been landed.  Closing bug.