sfnt table definitions

[bungeman, 2012-01-30 22:19:58]

The DirectWrite FontHost will need offsets into sfnt tables for font
information. Because of the size of this change, adding these tables has been
broken out into this separate review.

[reed1, 2012-01-31 13:11:16]

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkIBMFamilyClass.h
File include/sfnt/SkIBMFamilyClass.h (right):

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkIBMFamilyClass...
include/sfnt/SkIBMFamilyClass.h:16: struct SkIBMFamilyClass {
SkOTIBMFamilyClass?

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPanose.h
File include/sfnt/SkPanose.h (right):

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPanose.h#newco...
include/sfnt/SkPanose.h:15: 
SkOTPanose?

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPreprocessorSeq.h
File include/sfnt/SkPreprocessorSeq.h (right):

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPreprocessorSe...
include/sfnt/SkPreprocessorSeq.h:7: 
Ugh, a lot of defines without much name scoping. Maybe this is OK, but, for
example, SK_SIZE... does not denote macro-expansion-helper to me per se.

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPreprocessorSe...
include/sfnt/SkPreprocessorSeq.h:10: 
Where are SK_NIL and SK_NILL defined?

[bungeman, 2012-01-31 19:55:43]

Some reasons and excuses:

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkIBMFamilyClass.h
File include/sfnt/SkIBMFamilyClass.h (right):

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkIBMFamilyClass...
include/sfnt/SkIBMFamilyClass.h:16: struct SkIBMFamilyClass {
On 2012/01/31 13:11:16, reed1 wrote:
> SkOTIBMFamilyClass?

Panose has a much stronger argument for this, but this structure isn't specific
to OpenType. The weakness for this argument is that currently no one outside
OpenType uses it (and most fonts don't set it correctly anyway).

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPanose.h
File include/sfnt/SkPanose.h (right):

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPanose.h#newco...
include/sfnt/SkPanose.h:15: 
On 2012/01/31 13:11:16, reed1 wrote:
> SkOTPanose?

The OpenType spec, when it has attempted to define the Panose structure, has
always gotten it wrong. All of the modern specs now simply point to the Monotype
Panose specification. The Panose classification and structure actually are used
apart from OpenType in font design systems. If I did have a SkOTPanose it would
have to be the incorrect one listed in early versions of the OpenType spec which
disagrees with the data in many fonts (Impact, for example).

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPreprocessorSeq.h
File include/sfnt/SkPreprocessorSeq.h (right):

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPreprocessorSe...
include/sfnt/SkPreprocessorSeq.h:7: 
On 2012/01/31 13:11:16, reed1 wrote:
> Ugh, a lot of defines without much name scoping. Maybe this is OK, but, for
> example, SK_SIZE... does not denote macro-expansion-helper to me per se.

Hmmm... yes, SK_SIZE here should be SK_SEQ_SIZE. The SK_SEQ prefix could also be
longer, but I'm not sure what to make it.

http://codereview.appspot.com/5577064/diff/3037/include/sfnt/SkPreprocessorSe...
include/sfnt/SkPreprocessorSeq.h:10: 
On 2012/01/31 13:11:16, reed1 wrote:
> Where are SK_NIL and SK_NILL defined?

Ah, these should all be SK_NIL. They're actually placeholders to keep the
preprocessor happy and should never actually be evaluated, hence should never be
defined. It seemed shorter and logically clearer than SK_UNDEFINED or something
like that.

[bungeman, 2012-02-02 19:16:27]

Committed revision 3131.

[thakis, 2014-03-10 22:35:18]

clang complains about this file:

https://codereview.appspot.com/5577064/diff/11001/include/sfnt/SkOTTable_OS_2...
File include/sfnt/SkOTTable_OS_2_V1.h (right):

https://codereview.appspot.com/5577064/diff/11001/include/sfnt/SkOTTable_OS_2...
include/sfnt/SkOTTable_OS_2_V1.h:294: static const SK_OT_ULONG
GeneralPunctuationMask = SkTEndian_SwapBE32(1 << 31);
Shifting a 1 into a signed int is undefined behavior I think. Can you change
this to "1u << 31"?

https://codereview.appspot.com/5577064/diff/11001/include/sfnt/SkOTTable_OS_2...
include/sfnt/SkOTTable_OS_2_V1.h:487: static const SK_OT_ULONG
SymbolCharacterSetMask = SkTEndian_SwapBE32(1 << 31);
(here too)

[bungeman, 2014-03-10 23:13:55]

On 2014/03/10 22:35:18, thakis wrote:
> clang complains about this file:
> 
>
https://codereview.appspot.com/5577064/diff/11001/include/sfnt/SkOTTable_OS_2...
> File include/sfnt/SkOTTable_OS_2_V1.h (right):
> 
>
https://codereview.appspot.com/5577064/diff/11001/include/sfnt/SkOTTable_OS_2...
> include/sfnt/SkOTTable_OS_2_V1.h:294: static const SK_OT_ULONG
> GeneralPunctuationMask = SkTEndian_SwapBE32(1 << 31);
> Shifting a 1 into a signed int is undefined behavior I think. Can you change
> this to "1u << 31"?
> 
>
https://codereview.appspot.com/5577064/diff/11001/include/sfnt/SkOTTable_OS_2...
> include/sfnt/SkOTTable_OS_2_V1.h:487: static const SK_OT_ULONG
> SymbolCharacterSetMask = SkTEndian_SwapBE32(1 << 31);
> (here too)

Hmmm... yep, this is undefined with 32bit signed ints. Should probably make it
clear that all of these are unsigned shifts (in all of these files).

Not often one gets comments on two year old closed reviews :-) Interestingly
KDevelop with the clang parser highlights this with a problem marker, actually
complaining that the negative value won't fit into the uint_32. Looking for "<<
31" it looks like SkFloat and SkFloatBits may also have issues with this.

I have opened skia issue 2285 for the remaining instances and
https://codereview.chromium.org/189093020/ for this.