This looks dangerous. Why are these functions useful? It would help if we did some adoption in the same PR to understand how they'd be used and to make sure this is the best way.

createWithoutCopying is not lifetime safe. We should not expand usage, and we should probably work to remove it.

I wonder if lifetime compiler marks could help here.

If there were a lifetime mark here, the compiler would signal an error. The returned String has unbounded lifetime, so it always escapes the lifetime bound of any view type.

for (auto& [profileLevelId, spsAsInteger]: profiles)?

for (auto& [profileLevelId, spsAsInteger]: profiles)?

This should not be a vector.

Why do we want these functions to return a String rather than a CString or some other 8-bit string type? I don't think that's good. We don't need to make a hybrid 8/16-bit character string out of an 8-bit character string as a side effect of changing the capitalization of some characters. At least one of the callers just turns around and converts this back into a newly allocated ASCII string so it's allocating and destroying a String for no good reason.

These functions should work on spans rather than adding them all to CStringView; I suggest non-member functions. We would like them to work on 8-bit strings that don't have null terminators, too. The only member functions we should add to CStringView are ones that depend on the null termination. We don't just want extra versions of everything you can do to an 8-bit string in this class and then another copy for non-null-terminated strings.

If I underestood correctly, do you want me to move all comparison functions to be non member functions? Answering your other comment here as well: making them operate on just spans can be very inconvenient when you have to deal with this kind of strings a lot. Whenever I have to compare or convert, I have to get the span, then operate, then maybe get another result...

I've spoke about this with my mates of the GTK/WPE ports and we agree this class is useful. If you don't find it useful, I am open to move it to the glib directory, but we do need it. Just say the word.

Let me try to be explicit. You’re saying that this is useful:

x.containsOnlyASCII()

But this is very inconvenient?

containsOnlyASCII(x.span())

I think they are both really convenient!

I really hope we don’t have dueling style battles with people lined up along “port” lines.

I was not referring to that example only, that's obvious. I wrote that after putting my child to sleep without having time to give it a proper thought about other cases. I'll write the code today and see how it goes. I hope you're right! (And it usually happens).

Another concern is that I, as a developer, usually go to the header of a class to know if I can do certain operation. If we move all this out of the class, it will be "undocumented". I guess a comment in the code regarding being able to create the span and perform the same operations would be interesting.

I really hope we don’t have dueling style battles with people lined up along “port” lines.

It would be a nice team building activity for the next WKCM 😆

I think I addressed all changes you suggested to my understanding. I don't think I could detach startsWith, endsWith, find, reverseFind and their ignore case counterparts because they do rely on sizes and I haven't seen any other helpers in WTF I can base them on. Please let me know if you find this ok or you would like me to perform any other changes.

I think this isn't quite the right name for the concept because EqualityComparable sounds like it just answers whether two types support ==.

I tried to come up with one example of a better name: TriviallyComparableCodeUnits.

We should format new code consistently with the multiple line style used just above that we discussed in WebKit-dev or Slack. I don't think the name OneByteType is ideal given we already have a concept named ByteType that is almost identical. The difference between the two seems to be related to const and not the number one.

I think what you suggest is not possible because of error: non-const lvalue reference to type 'span<char>' cannot bind to a value of unrelated type 'span<char8_t>'. I'll keep this as it was.

Makes sense; my suggestion was wrong.

Do we need the byteCast<char>? I’m pretty sure a char can be assigned to a char8_t without a cast.

Yes I am still not sure why this comment is worded this way.

Why do we have to explicitly write String() here? Doesn't it compile without it?

These should be using EXPECT, not ASSERT. No idea why the existing code uses the wrong macro. The assert macros stop when the first one fails, which is not what we want in these tests.

Absolutely agree with you. However, I'd leave for some other patch because it would be a breakage of "commit atomicity".

It's not good for portability to rely on 0UL, which is unsigned long, being the same as size_t.

I have one doubt regarding this piece of code. It's really a pity to allocate a String when we could be living with just ASCIILiterals. The problem is that, regardless of if you change the parameter to make it ASCIILiteral, at some point you have to convert from CStringView to ASCIILiteral and that can't be done right now. Hence you need to convert to String instead.

One option would be turning the hashmap to store a CStringView built from an ASCIILiteral, but CStringView can't do hashes and I think it's a good idea because lifetime of the stored string could be a problem if people decide to use it with less care. I don't think this is a good idea.

Another option would be having a method CStringView::asASCIILiteral that checks if it's really ASCII and ASSERTing otherwise. We might handle something with LIFETIME_BOUND and maybe mitigate any wrong use of the asASCIILiteral method. I guess for this we would have to create another method in ASCIILiteral to be able to accept a span because ASCIILiteral::unsafeFromUTF8 does a pass to calculate the length of the string, which we already know.

None of these options makes me completely happy, neither converting to String, but I guess we need to take the best balancing safety and efficiency.

WDYT?

We should not try to convert a CStringView into an ASCIILiteral because UTF-8 isn't ASCII and there is no guarantee that the string is a literal. But I agree that it's not good to convert to String just to use a HashMap. For this kind of table we want to use a SortedArrayMap and we should make that work with the appropriate span type; it would be relatively straightforward. I think it’s a good idea to come back and deal with that later.

I swear I went through the whole patch trying to make it like this but I guess my eyes were faster than my brain.

This comment should mention the null terminator, which is the reason we don't just use span<const char8_t> for this.

The reason for this class is to let us pass around pointer to a null-terminated run of UTF-8 characters and have the type system document that it’s null-terminated.

We normally don't use the word method since C++ calls them functions or, more specifically, member functions.

I still remember what I learned in object oriented programming at university, learning Java, many years ago 😅

Not new to this pull request: Pretty sure this is wrong and will cause two empty strings to compare as non-equal if their data pointers don’t point to the same address. We should add tests and fix this.

I don't know now where I copied this from, but I am pretty sure it's somewhere else. I'll leave as it is in ASCIILiteral then, with just the equalSpans. I'll also add a test.

I added the test and, as usual, you were right!