freetype-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [ft-devel] New Infinality Release


From: Infinality
Subject: Re: [ft-devel] New Infinality Release
Date: Sat, 16 Jun 2012 10:56:21 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0

I'll certainly look into these things before committing the patches. I know you don't like floats, so I'll try to work around that. Regarding the formatting, some of those were oversights on my part. The thing with the "&&" way off at the end of the line was something I saw a precedent for somewhere else in existing Freetype code, so I thought it was "the standard". I too thought it looked odd, but figured you wanted it that way. ;)


On 06/16/2012 01:44 AM, Werner LEMBERG wrote:
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

_______________________________________________
Freetype-devel mailing list
address@hidden
https://lists.nongnu.org/mailman/listinfo/freetype-devel





reply via email to

[Prev in Thread] Current Thread [Next in Thread]