Bug 183165 – Delete incorrect version of clampTo() function from SVGToOTFFontConversion.cpp

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

In SVGToOTFFontConversion.cpp, there many calls to the template function clampTo(). The first few of these calls, call the clampTo() from MathExtras.h. But the rest of these calls call the static clampTo() in SVGToOTFFontConversion.cpp:

// Assumption: T2 can hold every value that a T1 can hold.
template<typename T1, typename T2> static inline T1 clampTo(T2 x)
{
    x = std::min(x, static_cast<T2>(std::numeric_limits<T1>::max()));
    x = std::max(x, static_cast<T2>(std::numeric_limits<T1>::min()));
    return static_cast<T1>(x);
}

If we delete this function, all the calls in SVGToOTFFontConversion.cpp will be directed to the clampTo() in MathExtras.h. The results of the clampTo() in MathExtras.h will match the result of the deleted function expect one call which is the following:

append16(clampTo<uint16_t>(m_weight)); // Weight class

The reason is m_weight is char and the static clampTo() always returns 0 while the clampTo in MathExtras.h returns the m_weight casted to uint16_t. Here is the reason why the local clampTo() always returns 0.

1. std::numeric_limits<uint16_t>::min() is 0
2. std::numeric_limits<uint16_t>::max() is 65535
3. static_cast<char>(std::numeric_limits<uint16_t>::max()) is 255 or -1
4. std::min(x, static_cast<char>(std::numeric_limits<uint16_t>::max())) is -1 if x >= 0, or x if x < 0
5. std::max(x, static_cast<char>(std::numeric_limits<uint16_t>::min())) is 0 since x is always < 0

[Said Abou-Hallawa]

Created attachment 334694 [details]
Patch

[Darin Adler]

(In reply to Said Abou-Hallawa from comment #0)
> append16(clampTo<uint16_t>(m_weight)); // Weight class
> 
> The reason is m_weight is char and the static clampTo() always returns 0
> while the clampTo in MathExtras.h returns the m_weight casted to uint16_t.

If m_weight is char, then on most platforms we support it will be signed. So returning m_weight casted to uint16_t would be incorrect for negative numbers. Is that really an accurate description of what the MathExtras.h version of clampTo does? Casting to uint16_t would be fine for unsigned char, but not for signed char. And using "char" seems like a mistake.

Can we add unit test cases to make sure clampTo works correctly?

Can we fix the font code to not use "char" for a numeric value, since whether it is signed or not is platform-dependent?

[Said Abou-Hallawa]

Created attachment 334975 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 334975 [details]
Patch

Clearing flags on attachment: 334975

Committed r229202: <https://trac.webkit.org/changeset/229202>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38111171>

[Said Abou-Hallawa]

(In reply to Darin Adler from comment #2)
> (In reply to Said Abou-Hallawa from comment #0)
> > append16(clampTo<uint16_t>(m_weight)); // Weight class
> > 
> > The reason is m_weight is char and the static clampTo() always returns 0
> > while the clampTo in MathExtras.h returns the m_weight casted to uint16_t.
> 
> If m_weight is char, then on most platforms we support it will be signed. So
> returning m_weight casted to uint16_t would be incorrect for negative
> numbers. Is that really an accurate description of what the MathExtras.h
> version of clampTo does? Casting to uint16_t would be fine for unsigned
> char, but not for signed char. And using "char" seems like a mistake.
> 

I agree.

-- I think the deleted version of clampTo() does not work correctly when clamping from a shorter signed integer to a longer unsigned integer e.g. char to uint16_t.

-- The version of clampTo() in MathExtras.h does not work correctly when clamping from shorter signed integer to longer unsigned integer but only if the integer is negative.

> Can we add unit test cases to make sure clampTo works correctly?
> 

I think both the deleted version and the existing versions of clampTo() will work correctly if they are used for clamping and not for casting. I found this statement really difficult to read 

append16(clampTo<uint16_t>(m_weight));

I could not figure out the intent of the clamping here when I found out m_weight is char. But most likely this code is for casting. We want to cast from signed char to uint16_t. I think it is better off if we wrote this statement like this:

append16(static_cast<uint16_t>(m_weight));

Which also will do the wrong casting if m_weight is negative. So I think the problem is not in clampTo(). The problem is in the code in the statement above and the code in SVGToOTFFontConverter::appendOS2Table().

> Can we fix the font code to not use "char" for a numeric value, since
> whether it is signed or not is platform-dependent?

I am not very familiar with the SVG font conversion. My purpose of this patch was to overcome a compilation error I got when I deleted the file svg/SVGAnimatedType.cpp in https://bugs.webkit.org/show_bug.cgi?id=183017. Deleting this files caused SVGToOTFFontConversion.cpp to be moved with a unified source file which has MathExtras.h included. I am filed this bug https://bugs.webkit.org/show_bug.cgi?id=183339 but I do not know the right way to fix it.

[Darin Adler]

(In reply to Said Abou-Hallawa from comment #7)
> -- The version of clampTo() in MathExtras.h does not work correctly when
> clamping from shorter signed integer to longer unsigned integer but only if
> the integer is negative.

Let’s add the test case for this to the unit tests.

> I found this statement really difficult to read 
> 
> append16(clampTo<uint16_t>(m_weight));
> 
> I could not figure out the intent of the clamping here when I found out
> m_weight is char. But most likely this code is for casting. We want to cast
> from signed char to uint16_t.

That’s wrong.

The clampTo function is supposed to convert out of range values to minimum and maximum values as appropriate. So it should make negative values zero. 

We could also make it fail to compile at all if no clamping is needed, like “clamping” from uint8_t to uint16_t, but I think it would be less useful for template programming if we did that.