Bug 183165 - Delete incorrect version of clampTo() function from SVGToOTFFontConversion.cpp
Summary: Delete incorrect version of clampTo() function from SVGToOTFFontConversion.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks: 183017
  Show dependency treegraph
 
Reported: 2018-02-27 10:24 PST by Said Abou-Hallawa
Modified: 2018-03-05 16:48 PST (History)
7 users (show)

See Also:


Attachments
Patch (8.70 KB, patch)
2018-02-27 10:39 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (8.71 KB, patch)
2018-03-03 20:24 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2018-02-27 10:24:43 PST
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
Comment 1 Said Abou-Hallawa 2018-02-27 10:39:39 PST
Created attachment 334694 [details]
Patch
Comment 2 Darin Adler 2018-02-28 14:35:31 PST
(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?
Comment 3 Said Abou-Hallawa 2018-03-03 20:24:20 PST
Created attachment 334975 [details]
Patch
Comment 4 WebKit Commit Bot 2018-03-03 21:19:17 PST
Comment on attachment 334975 [details]
Patch

Clearing flags on attachment: 334975

Committed r229202: <https://trac.webkit.org/changeset/229202>
Comment 5 WebKit Commit Bot 2018-03-03 21:19:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2018-03-03 21:20:21 PST
<rdar://problem/38111171>
Comment 7 Said Abou-Hallawa 2018-03-05 11:21:09 PST
(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.
Comment 8 Darin Adler 2018-03-05 16:48:26 PST
(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.