Re: [ft-devel] New Infinality Release

[Werner LEMBERG]

> For those interested, the latest release is available here:
> http://www.infinality.net/blog/infinality-freetype-patches/

Very nice!

Some remarks regarding
freetype-add-subpixel-hinting-infinality-20120615-01.patch:

  . In line 1440 I see this:

      +#endif /* TT_CONFIG_OPTION_SUBPIXEL_HINTING */
      +#ifdef TT_CONFIG_OPTION_SUBPIXEL_HINTING

    Just drop these too lines :-)

  . To get the distinction between GDI and DW ClearType (the former,
    older one doesn't support AA rendering along the y axis), Greg
    Hitchcock explained on the OpenType mailing list:

      Most likely, though, is the DirectWrite support for sub-pixel
      positioned ClearType.  This can be queried with GETINFO selector
      bit 10, return bit 17.  (In order to test this flag, the
      TrueType version must be >= 38 and <= 64.)  GDI does not support
      sub-pixel positioned ClearType.

    Bits 10/17, along with two other pairs, 11/18 and 12/19, lack
    complete documentation in the OpenType standard currently; Greg is
    going to fix that in the ClearType whitepaper, he told me.

    Shall we set bit pair 10/17 and use value 38 (not 37) for the
    rasterizer version?

  . I suggest to replace

      if ( distance < FT_MulDiv( minimum_distance_factor,
                                 CUR.GS.minimum_distance, 64 ) )
        distance = FT_MulDiv( minimum_distance_factor,
                              CUR.GS.minimum_distance, 64 );

    with

      new_distance = FT_MulDiv( minimum_distance_factor,
                                CUR.GS.minimum_distance, 64 );
      if ( distance < new_distance )
        distance = new_distance;

    to help dumb compilers.  I think it also makes the code more
    readable.  This is around lines 1300, 1310, 1404, and 1414 in the
    patch.

  . I'm not happy that you are using `float'.  Right now floating
    mathematics is not used in FreeType, and ideally I would like to
    stay with that (until someone is going to replace MulDiv
    completely).  Would it be a great burden to use FT_MulDiv and
    friends instead?

  . You use wide characters for Cyrillic, e.g.

      L'и'

    This is a bad idea.  First of all, MS compilers require them to be
    encoded in UTF-16 (which is only 16bit wide).  Second, as
    currently written in the source code, those values are encoded in
    a different encoding, namely UTF-8.  This doesn't fit.  Third, for
    higher Unicode values, UTF-8 gives three or more bytes which
    doesn't fit either.  Fourth, the correct type for L'...' would be
    wchar_t, but this is not portable.  Unicode 4.0 says:

      "The width of wchar_t is compiler-specific and can be as small
      as 8 bits.  Consequently, programs that need to be portable
      across any C or C++ compiler should not use wchar_t for storing
      Unicode text.  The wchar_t type is intended for storing
      compiler-defined wide characters, which may be Unicode
      characters in some compilers."

    Please replace them with numeric values (and a proper comment).

You did a lot of search and replace.  If possible I ask you to fix
some formatting stuff:

  . After the variable declarations within a block, you sometimes use
    a single line before the code starts.  I ask you to use two empty
    lines:

      FT_UInt  foo;


      foo = bar;

    And please avoid excessive, unnecessary whitespace before variable
    names:

      FT_UInt                 foo;

    Two spaces (if not aligning vertically) are fully sufficient for
    the weird FreeType formatting :-)

  . Please don't write

      if ( foo != 0                                    &&
           !( bar & baz ) )

    but rather

      if ( foo != 0       &&
           !( bar & baz ) )

    to align the left character of `&&' or `||' with the correct level
    of the closing parenthesis.  If necessary, move the closing brace
    to the right to get a nice vertical alignment.

  . Please say

      foo    = x;
      foobar = x;

    instead of

      foo = x;
      foobar = x;

  . Please always say

      if (foo)
        x = bar;
      else
        x = foobar;

    instead of

      if (foo) x = bar;
      else x = foobar;

    even for very short IF and IF-ELSE clauses.  I like vertical
    formatting a lot, and it improves legibility IMHO.

  . Please don't use braces for single-line blocks, thus

      if (foo)
      {
        foo = bar;
      }

    should rather be

      if (foo)
        foo = bar;

  . We have a space before a parenthesis only after a C
    keyword, but not after function or macro declarations:

      foo ( bar )    =>   foo( bar )

      sizeof( foo )  =>   sizeof ( foo )


   Werner