Bug 133592 – Use unsigneds instead of ints for indexes into a string in text layout code

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Myles C. Maxfield]

It would make more sense for it to return an unsigned.

[Myles C. Maxfield]

Created attachment 233210 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/17337617>

[Myles C. Maxfield]

Created attachment 233240 [details]
Patch

[WebKit Commit Bot]

Attachment 233240 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:106:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

*** Bug 133594 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

Comment on attachment 233240 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.cpp:340
> +    unsigned to = (initialTo == -1 ? run.length() : initialTo);

Can initialTo ever be -2 (or lower)? Should this be "initialTo < 0 ?", as it is in 'drawEmphasisMarks'?

> Source/WebCore/platform/graphics/Font.cpp:511
> +    unsigned to = (initialTo == -1 ? run.length() : initialTo);

Ditto above question.

> Source/WebCore/platform/graphics/Font.cpp:1068
> +    unsigned offsetInString = static_cast<unsigned>(initialOffsetInString);

What if initialOffsetInString is negative?

[Myles C. Maxfield]

Comment on attachment 233240 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.cpp:340
>> +    unsigned to = (initialTo == -1 ? run.length() : initialTo);
> 
> Can initialTo ever be -2 (or lower)? Should this be "initialTo < 0 ?", as it is in 'drawEmphasisMarks'?

It shouldn't be, but it's simple enough to change the conditional.

>> Source/WebCore/platform/graphics/Font.cpp:511
>> +    unsigned to = (initialTo == -1 ? run.length() : initialTo);
> 
> Ditto above question.

Same answer.

>> Source/WebCore/platform/graphics/Font.cpp:1068
>> +    unsigned offsetInString = static_cast<unsigned>(initialOffsetInString);
> 
> What if initialOffsetInString is negative?

It should never be - the only negative value that it should ever have is -1 (kNoOffset). I'll add an assert for this.

[Myles C. Maxfield]

Created attachment 233253 [details]
Patch

[WebKit Commit Bot]

Attachment 233253 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:106:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

The style red bubble is a false negative, and it looks like the EFL bot was killed during compilation

[alan]

Comment on attachment 233253 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:202
> +    if (endPos < m_start)

if (endPos <= m_start)

> Source/WebCore/rendering/InlineTextBox.cpp:204
> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));

Could you change m_len's type to unsigned too?
I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.

Its setter looks wrong too and can result in unsigned overflow:
void setLen(unsigned len) { m_len = len; }
So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.

It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.

> Source/WebCore/rendering/InlineTextBox.cpp:278
> +    if (endPos < m_start)

same here. though now that I am seeing it the second time, it might be better to keep the old structure, though I don't feel too strongly about it.  (shaving off a std::min() call vs. better code structuring (better code reading, less error prone))

[alan]

Comment on attachment 233253 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:202
> +    if (endPos < m_start)

if (endPos <= m_start)

> Source/WebCore/rendering/InlineTextBox.cpp:204
> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));

Could you change m_len's type to unsigned too?
I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.

Its setter looks wrong too and can result in unsigned overflow:
void setLen(unsigned len) { m_len = len; }
So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.

It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.

> Source/WebCore/rendering/InlineTextBox.cpp:278
> +    if (endPos < m_start)

same here. though now that I am seeing it the second time, it might be better to keep the old structure, though I don't feel too strongly about it.  (shaving off a std::min() call vs. better code structuring (better code reading, less error prone))

[Myles C. Maxfield]

Comment on attachment 233253 [details]
Patch

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

>>> Source/WebCore/rendering/InlineTextBox.cpp:204
>>> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));
>> 
>> Could you change m_len's type to unsigned too?
>> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
>> 
>> Its setter looks wrong too and can result in unsigned overflow:
>> void setLen(unsigned len) { m_len = len; }
>> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
>> 
>> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> 
> Could you change m_len's type to unsigned too?
> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> 
> Its setter looks wrong too and can result in unsigned overflow:
> void setLen(unsigned len) { m_len = len; }
> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> 
> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.

That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics.

[alan]

(In reply to comment #13)
> (From update of attachment 233253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233253&action=review
> 
> >>> Source/WebCore/rendering/InlineTextBox.cpp:204
> >>> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));
> >> 
> >> Could you change m_len's type to unsigned too?
> >> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> >> 
> >> Its setter looks wrong too and can result in unsigned overflow:
> >> void setLen(unsigned len) { m_len = len; }
> >> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> >> 
> >> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> > 
> > Could you change m_len's type to unsigned too?
> > I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> > 
> > Its setter looks wrong too and can result in unsigned overflow:
> > void setLen(unsigned len) { m_len = len; }
> > So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> > 
> > It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> 
> That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics.
On certain hw, going from signed to unsigned does change runtime characteristic, but I agree, it's better to do it in a separate patch.

[Myles C. Maxfield]

Created attachment 233476 [details]
Patch

[Myles C. Maxfield]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 233253 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=233253&action=review
> > 
> > >>> Source/WebCore/rendering/InlineTextBox.cpp:204
> > >>> +    unsigned ePos = std::min(endPos - m_start, static_cast<unsigned>(m_len));
> > >> 
> > >> Could you change m_len's type to unsigned too?
> > >> I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> > >> 
> > >> Its setter looks wrong too and can result in unsigned overflow:
> > >> void setLen(unsigned len) { m_len = len; }
> > >> So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> > >> 
> > >> It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> > > 
> > > Could you change m_len's type to unsigned too?
> > > I talked to Kling about it and while it was originally typed to be short to save memory, now with the simple line layout, we don't have that excessive amount of inlinetextboxes so it's ok to make it unsigned int.
> > > 
> > > Its setter looks wrong too and can result in unsigned overflow:
> > > void setLen(unsigned len) { m_len = len; }
> > > So it's either the callers start using unsigned shorts or we make m_len unsigned int. -also, it's a bit odd that most of the inlinetextbox functions operate on ints while the length is short.
> > > 
> > > It might be better though to have this m_len change in a separate patch. If it happens to bring in some major memory regression, we can easily revert it without undoing the signed-unsigned change.
> > 
> > That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics.
> On certain hw, going from signed to unsigned does change runtime characteristic, but I agree, it's better to do it in a separate patch.

Really? Signed math isn't always the same speed as unsigned math on all hardware? I had no idea.

[WebKit Commit Bot]

Attachment 233476 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:106:  The parameter name "const" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

(In reply to comment #16)
> > > That's a good idea, but I do thing that that would be better to do in another patch. I designed this one to have no changed runtime characteristics.
> > On certain hw, going from signed to unsigned does change runtime characteristic, but I agree, it's better to do it in a separate patch.
> 
> Really? Signed math isn't always the same speed as unsigned math on all hardware? I had no idea.

ARMv7 doesn't have an instruction for signed modulo, for instance.

[alan]

Comment on attachment 233476 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:278
> +    if (endPos < m_start)

endPos <= m_start

[Myles C. Maxfield]

http://trac.webkit.org/changeset/170265

[WebKit Commit Bot]

Re-opened since this is blocked by bug 134174

[Myles C. Maxfield]

The problem was a "<" vs a "<=". Re-landed in http://trac.webkit.org/changeset/170337.

[Csaba Osztrogonác]

(In reply to comment #22)
> The problem was a "<" vs a "<=". Re-landed in http://trac.webkit.org/changeset/170337.

It made 3 tests crash on Apple Mac debug bots.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 134250

[Myles C. Maxfield]

Having trouble reproducing locally :(

[Myles C. Maxfield]

Was able to reproduce these, and the fix is trivial. I'll land again shortly. Hopefully this time it sticks.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/170390