Make AtomicMarkupTokenBase use a bare UChar* for external characters

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Gentilcore]

Make AtomicMarkupTokenBase use a bare UChar* for external characters

[Tony Gentilcore]

Created attachment 182800 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 182800 [details]
Patch

ok.

[WebKit Review Bot]

Comment on attachment 182800 [details]
Patch

Clearing flags on attachment: 182800

Committed r139760: <http://trac.webkit.org/changeset/139760>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

Comment on attachment 182800 [details]
Patch

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

> Source/WebCore/xml/parser/MarkupTokenBase.h:529
> +    size_t charactersLength() const
> +    {
> +        ASSERT(m_type == Token::Type::Character);
> +        return m_externalCharactersLength;
>      }

While you are at it: this would be better as unsigned instead of size_t.
Character buffers are usually unsigned (WTFString included).

> Source/WebCore/xml/parser/MarkupTokenBase.h:588
> +    size_t m_externalCharactersLength;

ditto.

[Tony Gentilcore]

Comment on attachment 182800 [details]
Patch

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

>> Source/WebCore/xml/parser/MarkupTokenBase.h:529
>>      }
> 
> While you are at it: this would be better as unsigned instead of size_t.
> Character buffers are usually unsigned (WTFString included).

abarth tells me that on a 64bit system, unsigned will still be 32 bits but size_t will be 64. Since Vectors are arbitrary length, using size_t is appropriate here.

[Adam Barth]

I wonder if we should crash if we get a character token that's larger than 32 bits.  I bet we do later anyway due to overflow checks in StringImpl.

[Benjamin Poulain]

> abarth tells me that on a 64bit system, unsigned will still be 32 bits but size_t will be 64. 

That is correct.

> Since Vectors are arbitrary length, using size_t is appropriate here.

Well, that's all nice when creating the Token, but the method you call will get you un trouble. E.g.:
    String characters = String(token->characters(), token->charactersLength());

Your patch changed from using Vector types to Character types, which is why I suggest we move to string conventions.


> I wonder if we should crash if we get a character token that's larger than 32 bits.  I bet we do later anyway due to overflow checks in StringImpl.

Yep, I think you are right.
Seeing what is here, I think that's the way to make this correct.

[Adam Barth]

Yeah.  Would you be willing to file a bug about this issue?  The issue pre-dates this patch since the old code used Vector::size as well.