Re: harfbuzz 2f72162: Fix crash in the Cairo build

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

eliz@gnu.org (Eli Zaretskii) writes:

> diff --git a/src/ftcrfont.c b/src/ftcrfont.c
> index e2f84d4..6d74d93 100644
> --- a/src/ftcrfont.c
> +++ b/src/ftcrfont.c
> @@ -41,6 +41,9 @@ struct ftcrfont_info
>    bool maybe_otf;	  /* Flag to tell if this may be OTF or not.  */
>    OTF *otf;
>  #endif	/* HAVE_LIBOTF */
> +#ifdef HAVE_HARFBUZZ
> +  hb_font_t *hb_font;
> +#endif  /* HAVE_HARFBUZZ */
>    FT_Size ft_size;
>    int index;
>    FT_Matrix matrix;

Eli, what would be your preferred type of solution to avoid this in
future? A #define of the common portion of the structs in ftfont.h? An
embedded 'struct ft_font_fixed' member inside struct ft{,cr}font_info?
Something else? At the very least I propose syncing the comments:

diff --git i/src/ftcrfont.c w/src/ftcrfont.c
index e2f84d44fc..7d63a344ec 100644
--- i/src/ftcrfont.c
+++ w/src/ftcrfont.c
@@ -35,12 +35,16 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 struct ftcrfont_info
 {
   struct font font;
-  /* The following six members must be here in this order to be
-     compatible with struct ftfont_info (in ftfont.c).  */
+  /* The following members up to and including 'matrix' must be here
+     in this order to be compatible with struct ftfont_info (in
+     ftfont.c).  */
 #ifdef HAVE_LIBOTF
   bool maybe_otf;	  /* Flag to tell if this may be OTF or not.  */
   OTF *otf;
 #endif	/* HAVE_LIBOTF */
+#ifdef HAVE_HARFBUZZ
+  hb_font_t *hb_font;
+#endif  /* HAVE_HARFBUZZ */
   FT_Size ft_size;
   int index;
   FT_Matrix matrix;
diff --git i/src/ftfont.c w/src/ftfont.c
index a645bbf029..ef4ccab3ec 100644
--- i/src/ftfont.c
+++ w/src/ftfont.c
@@ -56,8 +56,9 @@ struct ftfont_info
 {
   struct font font;
 #ifdef HAVE_LIBOTF
-  /* The following four members must be here in this order to be
-     compatible with struct xftfont_info (in xftfont.c).  */
+  /* The following members up to and including 'matrix' must be here in
+     this order to be compatible with struct xftfont_info (in
+     xftfont.c).  */
   bool maybe_otf;	/* Flag to tell if this may be OTF or not.  */
   OTF *otf;
 #endif	/* HAVE_LIBOTF */

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Fri, 14 Dec 2018 13:18:41 +0100
> Cc: Ari Roponen <ari.roponen@gmail.com>
> 
> Eli, what would be your preferred type of solution to avoid this in
> future?

If at all possible, I prefer a single definition of each struct, with
as few #ifdef's as possible.  If some member is not used in some
configuration, I prefer to have it anyway, unless it cannot be
compiled in the configuration(s) that don't use it (e.g., because it
uses a data type that is only visible in some configurations).

> At the very least I propose syncing the comments:

Yes, please push.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Fri, 14 Dec 2018 13:18:41 +0100
>> Cc: Ari Roponen <ari.roponen@gmail.com>
>> 
>> Eli, what would be your preferred type of solution to avoid this in
>> future?
>
> If at all possible, I prefer a single definition of each struct, with
> as few #ifdef's as possible.  If some member is not used in some
> configuration, I prefer to have it anyway, unless it cannot be
> compiled in the configuration(s) that don't use it (e.g., because it
> uses a data type that is only visible in some configurations).
>

This I will do in master.

>> At the very least I propose syncing the comments:
>
> Yes, please push.

This I pushed to emacs-26 as f14d5742db

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Fri, 14 Dec 2018 13:18:41 +0100
>> Cc: Ari Roponen <ari.roponen@gmail.com>
>> 
>> Eli, what would be your preferred type of solution to avoid this in
>> future?
>
> If at all possible, I prefer a single definition of each struct, with
> as few #ifdef's as possible.  If some member is not used in some
> configuration, I prefer to have it anyway, unless it cannot be
> compiled in the configuration(s) that don't use it (e.g., because it
> uses a data type that is only visible in some configurations).
>

Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
try to unify xftfont.c in there as well? Iʼm tempted to get rid of
ftxfont.c as well, unless someone is actually still using that. [1]

Robert

Footnotes:
[1] How many different font backends can we come up with using only 3
     letters?

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org,  ari.roponen@gmail.com
> Date: Fri, 14 Dec 2018 17:16:46 +0100
> 
> Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
> try to unify xftfont.c in there as well?

If that makes sense, i.e. if the commonality seems to justify that,
then yes.

> Iʼm tempted to get rid of ftxfont.c as well, unless someone is
> actually still using that. [1]

I think this is a decision for another time.  In any case, we cannot
just remove it, we need to deprecate it first.  We may declare it
deprecated now, and perhaps cause its inclusion to trigger some
deprecation message during building Emacs.

> [1] How many different font backends can we come up with using only 3
>      letters?

That calls for a Lisp program, no?

Thanks.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: emacs-devel@gnu.org,  ari.roponen@gmail.com
>> Date: Fri, 14 Dec 2018 17:16:46 +0100
>> 
>> Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
>> try to unify xftfont.c in there as well?
>
> If that makes sense, i.e. if the commonality seems to justify that,
> then yes.
>

I had a quick go at this. Iʼve not measured the memory difference, but
I can do if people thinks this refactor is worth it. Apart from the
unification of the three struct definitions, most of the patch is
mechanical changes.

 src/ftcrfont.c | 41 ++++++-----------------------------------
 src/ftfont.c   | 58 +++++++++++++++++++++++++---------------------------------
 src/ftfont.h   | 28 +++++++++++++++++++++++++++-
 src/xftfont.c  | 43 +++++++++++--------------------------------
 4 files changed, 69 insertions(+), 101 deletions(-)

diff --git i/src/ftcrfont.c w/src/ftcrfont.c
index dc1a389c60..3c963e0e71 100644
--- i/src/ftcrfont.c
+++ w/src/ftcrfont.c
@@ -27,32 +27,6 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include "font.h"
 #include "ftfont.h"
 
-/* FTCR font driver.  */
-
-/* The actual structure for FTCR font.  A pointer to this structure
-   can be cast to struct font *.  */
-
-struct ftcrfont_info
-{
-  struct font font;
-  /* The following six members must be here in this order to be
-     compatible with struct ftfont_info (in ftfont.c).  */
-#ifdef HAVE_LIBOTF
-  bool maybe_otf;	  /* Flag to tell if this may be OTF or not.  */
-  OTF *otf;
-#endif	/* HAVE_LIBOTF */
-  FT_Size ft_size;
-  int index;
-  FT_Matrix matrix;
-
-  cairo_font_face_t *cr_font_face;
-  /* To prevent cairo from cluttering the activated FT_Size maintained
-     in ftfont.c, we activate this special FT_Size before drawing.  */
-  FT_Size ft_size_draw;
-  /* Font metrics cache.  */
-  struct font_metrics **metrics;
-  short metrics_nrows;
-};
 
 #define METRICS_NCOLS_PER_ROW	(128)
 
@@ -70,7 +44,7 @@ ftcrfont_glyph_extents (struct font *font,
                         unsigned glyph,
                         struct font_metrics *metrics)
 {
-  struct ftcrfont_info *ftcrfont_info = (struct ftcrfont_info *) font;
+  struct font_info *ftcrfont_info = (struct font_info *) font;
   int row, col;
   struct font_metrics *cache;
 
@@ -132,7 +106,7 @@ ftcrfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
 {
   Lisp_Object font_object;
   struct font *font;
-  struct ftcrfont_info *ftcrfont_info;
+  struct font_info *ftcrfont_info;
   FT_Face ft_face;
   FT_UInt size;
 
@@ -140,14 +114,14 @@ ftcrfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   size = XFIXNUM (AREF (entity, FONT_SIZE_INDEX));
   if (size == 0)
     size = pixel_size;
-  font_object = font_build_object (VECSIZE (struct ftcrfont_info),
+  font_object = font_build_object (VECSIZE (struct font_info),
 				   Qftcr, entity, size);
   font_object = ftfont_open2 (f, entity, pixel_size, font_object);
   if (NILP (font_object)) return Qnil;
 
   font = XFONT_OBJECT (font_object);
   font->driver = &ftcrfont_driver;
-  ftcrfont_info = (struct ftcrfont_info *) font;
+  ftcrfont_info = (struct font_info *) font;
   ft_face = ftcrfont_info->ft_size->face;
   FT_New_Size (ft_face, &ftcrfont_info->ft_size_draw);
   FT_Activate_Size (ftcrfont_info->ft_size_draw);
@@ -167,7 +141,7 @@ ftcrfont_close (struct font *font)
   if (font_data_structures_may_be_ill_formed ())
     return;
 
-  struct ftcrfont_info *ftcrfont_info = (struct ftcrfont_info *) font;
+  struct font_info *ftcrfont_info = (struct font_info *) font;
   int i;
 
   block_input ();
@@ -223,7 +197,7 @@ ftcrfont_draw (struct glyph_string *s,
 {
   struct frame *f = s->f;
   struct face *face = s->face;
-  struct ftcrfont_info *ftcrfont_info = (struct ftcrfont_info *) s->font;
+  struct font_info *ftcrfont_info = (struct font_info *) s->font;
   cairo_t *cr;
   cairo_glyph_t *glyphs;
   cairo_surface_t *surface;
@@ -312,9 +286,6 @@ struct font_driver const ftcrfont_driver =
 void
 syms_of_ftcrfont (void)
 {
-  if (ftfont_info_size != offsetof (struct ftcrfont_info, cr_font_face))
-    abort ();
-
   DEFSYM (Qftcr, "ftcr");
   register_font_driver (&ftcrfont_driver, NULL);
 }
diff --git i/src/ftfont.c w/src/ftfont.c
index e83eff3ad0..5d4fcc4a45 100644
--- i/src/ftfont.c
+++ w/src/ftfont.c
@@ -24,6 +24,17 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <fontconfig/fontconfig.h>
 #include <fontconfig/fcfreetype.h>
 
+/* These two blocks are here because this file is built when using XFT
+   and when using Cairo, so struct font_info in ftfont.h needs access
+   to the appropriate types.  */
+#ifdef HAVE_XFT
+#include <X11/Xlib.h>
+#include <X11/Xft/Xft.h>
+#endif
+#ifdef USE_CAIRO
+#include <cairo-ft.h>
+#endif
+
 #include <c-strcase.h>
 
 #include "lisp.h"
@@ -49,25 +60,6 @@ static Lisp_Object freetype_font_cache;
 /* Cache for FT_Face and FcCharSet. */
 static Lisp_Object ft_face_cache;
 
-/* The actual structure for FreeType font that can be cast to struct
-   font.  */
-
-struct ftfont_info
-{
-  struct font font;
-#ifdef HAVE_LIBOTF
-  /* The following four members must be here in this order to be
-     compatible with struct xftfont_info (in xftfont.c).  */
-  bool maybe_otf;	/* Flag to tell if this may be OTF or not.  */
-  OTF *otf;
-#endif	/* HAVE_LIBOTF */
-  FT_Size ft_size;
-  int index;
-  FT_Matrix matrix;
-};
-
-size_t ftfont_info_size = sizeof (struct ftfont_info);
-
 enum ftfont_cache_for
   {
     FTFONT_CACHE_FOR_FACE,
@@ -452,7 +444,7 @@ ftfont_get_fc_charset (Lisp_Object entity)
 
 #ifdef HAVE_LIBOTF
 static OTF *
-ftfont_get_otf (struct ftfont_info *ftfont_info)
+ftfont_get_otf (struct font_info *ftfont_info)
 {
   OTF *otf;
 
@@ -1095,7 +1087,7 @@ ftfont_open2 (struct frame *f,
               int pixel_size,
               Lisp_Object font_object)
 {
-  struct ftfont_info *ftfont_info;
+  struct font_info *ftfont_info;
   struct font *font;
   struct ftfont_cache_data *cache_data;
   FT_Face ft_face;
@@ -1146,7 +1138,7 @@ ftfont_open2 (struct frame *f,
 
   ASET (font_object, FONT_FILE_INDEX, filename);
   font = XFONT_OBJECT (font_object);
-  ftfont_info = (struct ftfont_info *) font;
+  ftfont_info = (struct font_info *) font;
   ftfont_info->ft_size = ft_face->size;
   ftfont_info->index = XFIXNUM (idx);
 #ifdef HAVE_LIBOTF
@@ -1236,7 +1228,7 @@ ftfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   size = XFIXNUM (AREF (entity, FONT_SIZE_INDEX));
   if (size == 0)
     size = pixel_size;
-  font_object = font_build_object (VECSIZE (struct ftfont_info),
+  font_object = font_build_object (VECSIZE (struct font_info),
 				   Qfreetype, entity, size);
   return ftfont_open2 (f, entity, pixel_size, font_object);
 }
@@ -1247,7 +1239,7 @@ ftfont_close (struct font *font)
   if (font_data_structures_may_be_ill_formed ())
     return;
 
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   Lisp_Object val, cache;
 
   val = Fcons (font->props[FONT_FILE_INDEX], make_fixnum (ftfont_info->index));
@@ -1291,9 +1283,9 @@ ftfont_has_char (Lisp_Object font, int c)
     }
   else
     {
-      struct ftfont_info *ftfont_info;
+      struct font_info *ftfont_info;
 
-      ftfont_info = (struct ftfont_info *) XFONT_OBJECT (font);
+      ftfont_info = (struct font_info *) XFONT_OBJECT (font);
       return (FT_Get_Char_Index (ftfont_info->ft_size->face, (FT_ULong) c)
 	      != 0);
     }
@@ -1302,7 +1294,7 @@ ftfont_has_char (Lisp_Object font, int c)
 unsigned
 ftfont_encode_char (struct font *font, int c)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   FT_Face ft_face = ftfont_info->ft_size->face;
   FT_ULong charcode = c;
   FT_UInt code = FT_Get_Char_Index (ft_face, charcode);
@@ -1314,7 +1306,7 @@ void
 ftfont_text_extents (struct font *font, unsigned int *code,
 		     int nglyphs, struct font_metrics *metrics)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   FT_Face ft_face = ftfont_info->ft_size->face;
   int i, width = 0;
   bool first;
@@ -1357,7 +1349,7 @@ ftfont_text_extents (struct font *font, unsigned int *code,
 int
 ftfont_get_bitmap (struct font *font, unsigned int code, struct font_bitmap *bitmap, int bits_per_pixel)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   FT_Face ft_face = ftfont_info->ft_size->face;
   FT_Int32 load_flags = FT_LOAD_RENDER;
 
@@ -1401,7 +1393,7 @@ int
 ftfont_anchor_point (struct font *font, unsigned int code, int idx,
 		     int *x, int *y)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   FT_Face ft_face = ftfont_info->ft_size->face;
 
   if (ftfont_info->ft_size != ft_face->size)
@@ -1466,7 +1458,7 @@ ftfont_otf_features (OTF_GSUB_GPOS *gsub_gpos)
 Lisp_Object
 ftfont_otf_capability (struct font *font)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   OTF *otf = ftfont_get_otf (ftfont_info);
   Lisp_Object gsub_gpos;
 
@@ -2616,7 +2608,7 @@ Lisp_Object
 ftfont_shape (Lisp_Object lgstring)
 {
   struct font *font = CHECK_FONT_GET_OBJECT (LGSTRING_FONT (lgstring));
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   OTF *otf = ftfont_get_otf (ftfont_info);
 
   return ftfont_shape_by_flt (lgstring, font, ftfont_info->ft_size->face, otf,
@@ -2630,7 +2622,7 @@ ftfont_shape (Lisp_Object lgstring)
 int
 ftfont_variation_glyphs (struct font *font, int c, unsigned variations[256])
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   OTF *otf = ftfont_get_otf (ftfont_info);
 
   if (! otf)
diff --git i/src/ftfont.h w/src/ftfont.h
index 4201b2c2d6..1f3416d2d2 100644
--- i/src/ftfont.h
+++ w/src/ftfont.h
@@ -41,6 +41,32 @@ extern Lisp_Object ftfont_open2 (struct frame *f,
                                  Lisp_Object entity,
                                  int pixel_size,
                                  Lisp_Object font_object);
-extern size_t ftfont_info_size;
+
+struct font_info
+{
+  struct font font;
+#ifdef HAVE_LIBOTF
+  bool maybe_otf;	/* Flag to tell if this may be OTF or not.  */
+  OTF *otf;
+#endif	/* HAVE_LIBOTF */
+  FT_Size ft_size;
+  int index;
+  FT_Matrix matrix;
+
+#ifdef USE_CAIRO
+  cairo_font_face_t *cr_font_face;
+  /* To prevent cairo from cluttering the activated FT_Size maintained
+     in ftfont.c, we activate this special FT_Size before drawing.  */
+  FT_Size ft_size_draw;
+  /* Font metrics cache.  */
+  struct font_metrics **metrics;
+  short metrics_nrows;
+#else
+  /* These are used by the XFT backend.  */
+  Display *display;
+  XftFont *xftfont;
+  unsigned x_display_id;
+#endif
+};
 
 #endif	/* EMACS_FTFONT_H */
diff --git i/src/xftfont.c w/src/xftfont.c
index 85df0d857a..764862aa3a 100644
--- i/src/xftfont.c
+++ w/src/xftfont.c
@@ -35,27 +35,6 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 /* Xft font driver.  */
 
-
-/* The actual structure for Xft font that can be cast to struct
-   font.  */
-
-struct xftfont_info
-{
-  struct font font;
-  /* The following five members must be here in this order to be
-     compatible with struct ftfont_info (in ftfont.c).  */
-#ifdef HAVE_LIBOTF
-  bool maybe_otf;	  /* Flag to tell if this may be OTF or not.  */
-  OTF *otf;
-#endif	/* HAVE_LIBOTF */
-  FT_Size ft_size;
-  int index;
-  FT_Matrix matrix;
-  Display *display;
-  XftFont *xftfont;
-  unsigned x_display_id;
-};
-
 /* Structure pointed by (struct face *)->extra  */
 
 struct xftface_info
@@ -255,7 +234,7 @@ xftfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   Display *display = FRAME_X_DISPLAY (f);
   Lisp_Object val, filename, idx, font_object;
   FcPattern *pat = NULL, *match;
-  struct xftfont_info *xftfont_info = NULL;
+  struct font_info *xftfont_info = NULL;
   struct font *font;
   double size = 0;
   XftFont *xftfont = NULL;
@@ -330,7 +309,7 @@ xftfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
 
   /* We should not destroy PAT here because it is kept in XFTFONT and
      destroyed automatically when XFTFONT is closed.  */
-  font_object = font_build_object (VECSIZE (struct xftfont_info),
+  font_object = font_build_object (VECSIZE (struct font_info),
 				   Qxft, entity, size);
   ASET (font_object, FONT_FILE_INDEX, filename);
   font = XFONT_OBJECT (font_object);
@@ -338,7 +317,7 @@ xftfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   font->driver = &xftfont_driver;
   font->encoding_charset = font->repertory_charset = -1;
 
-  xftfont_info = (struct xftfont_info *) font;
+  xftfont_info = (struct font_info *) font;
   xftfont_info->display = display;
   xftfont_info->xftfont = xftfont;
   xftfont_info->x_display_id = FRAME_DISPLAY_INFO (f)->x_id;
@@ -460,7 +439,7 @@ static void
 xftfont_close (struct font *font)
 {
   struct x_display_info *xdi;
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) font;
+  struct font_info *xftfont_info = (struct font_info *) font;
 
 #ifdef HAVE_LIBOTF
   if (xftfont_info->otf)
@@ -526,7 +505,7 @@ xftfont_done_face (struct frame *f, struct face *face)
 static int
 xftfont_has_char (Lisp_Object font, int c)
 {
-  struct xftfont_info *xftfont_info;
+  struct font_info *xftfont_info;
   struct charset *cs = NULL;
 
   if (EQ (AREF (font, FONT_ADSTYLE_INDEX), Qja)
@@ -540,7 +519,7 @@ xftfont_has_char (Lisp_Object font, int c)
 
   if (FONT_ENTITY_P (font))
     return ftfont_has_char (font, c);
-  xftfont_info = (struct xftfont_info *) XFONT_OBJECT (font);
+  xftfont_info = (struct font_info *) XFONT_OBJECT (font);
   return (XftCharExists (xftfont_info->display, xftfont_info->xftfont,
 			 (FcChar32) c) == FcTrue);
 }
@@ -548,7 +527,7 @@ xftfont_has_char (Lisp_Object font, int c)
 static unsigned
 xftfont_encode_char (struct font *font, int c)
 {
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) font;
+  struct font_info *xftfont_info = (struct font_info *) font;
   unsigned code = XftCharIndex (xftfont_info->display, xftfont_info->xftfont,
 				(FcChar32) c);
 
@@ -559,7 +538,7 @@ static void
 xftfont_text_extents (struct font *font, unsigned int *code,
 		      int nglyphs, struct font_metrics *metrics)
 {
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) font;
+  struct font_info *xftfont_info = (struct font_info *) font;
   XGlyphInfo extents;
 
   block_input ();
@@ -601,7 +580,7 @@ xftfont_draw (struct glyph_string *s, int from, int to, int x, int y,
 
   struct frame *f = s->f;
   struct face *face = s->face;
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) s->font;
+  struct font_info *xftfont_info = (struct font_info *) s->font;
   struct xftface_info *xftface_info = NULL;
   XftDraw *xft_draw = xftfont_get_xft_draw (f);
   FT_UInt *code;
@@ -664,7 +643,7 @@ static Lisp_Object
 xftfont_shape (Lisp_Object lgstring)
 {
   struct font *font = CHECK_FONT_GET_OBJECT (LGSTRING_FONT (lgstring));
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) font;
+  struct font_info *xftfont_info = (struct font_info *) font;
   FT_Face ft_face = XftLockFace (xftfont_info->xftfont);
   xftfont_info->ft_size = ft_face->size;
   Lisp_Object val = ftfont_shape (lgstring);
@@ -708,7 +687,7 @@ static bool
 xftfont_cached_font_ok (struct frame *f, Lisp_Object font_object,
                         Lisp_Object entity)
 {
-  struct xftfont_info *info = (struct xftfont_info *) XFONT_OBJECT (font_object);
+  struct font_info *info = (struct font_info *) XFONT_OBJECT (font_object);
   FcPattern *oldpat = info->xftfont->pattern;
   Display *display = FRAME_X_DISPLAY (f);
   FcPattern *pat = FcPatternCreate ();

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org,  ari.roponen@gmail.com
> Date: Mon, 17 Dec 2018 13:41:11 +0100
> 
> >> Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
> >> try to unify xftfont.c in there as well?
> >
> > If that makes sense, i.e. if the commonality seems to justify that,
> > then yes.
> >
> 
> I had a quick go at this. Iʼve not measured the memory difference, but
> I can do if people thinks this refactor is worth it. Apart from the
> unification of the three struct definitions, most of the patch is
> mechanical changes.

Thanks, this LGTM, and is a significant improvement, IMO.  If no one
objects in a few days, please push.

P.S.  Will this have any problems with the harfbuzz branch?

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: emacs-devel@gnu.org,  ari.roponen@gmail.com
>> Date: Mon, 17 Dec 2018 13:41:11 +0100
>> 
>> >> Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
>> >> try to unify xftfont.c in there as well?
>> >
>> > If that makes sense, i.e. if the commonality seems to justify that,
>> > then yes.
>> >
>> 
>> I had a quick go at this. Iʼve not measured the memory difference, but
>> I can do if people thinks this refactor is worth it. Apart from the
>> unification of the three struct definitions, most of the patch is
>> mechanical changes.
>
> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
> objects in a few days, please push.
>
> P.S.  Will this have any problems with the harfbuzz branch?

Apart from having to adjust the addition to the struct in question
done on the harfbuzz branch, I donʼt think so. Probably there will be
a merge conflict, I can help out with that if needed.

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Robert Pluim <rpluim@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>>> I had a quick go at this. Iʼve not measured the memory difference, but
>>> I can do if people thinks this refactor is worth it. Apart from the
>>> unification of the three struct definitions, most of the patch is
>>> mechanical changes.
>>

Given that my cairo build of emacs opens only 4 fonts on startup, and
the freetype one only 2, I think the memory issue is nonexistent.

>> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
>> objects in a few days, please push.
>>
>> P.S.  Will this have any problems with the harfbuzz branch?
>
> Apart from having to adjust the addition to the struct in question
> done on the harfbuzz branch, I donʼt think so. Probably there will be
> a merge conflict, I can help out with that if needed.

Having looked closer, there might be a few more conflicts, but nothing
too complicated to handle. Mainly a few instances of struct
ftfont_info -> struct font_info

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:
>
> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
> objects in a few days, please push.

I built this on GNU/Linux with the following configs:

./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
./configure --with-cairo   # This builds ftfont.o ftcrfont.o
./configure --without-xft  # This builds xfont.o only

Pushed to emacs-26 as 

Just kidding. Pushed to master as

I can cherry-pick this to the harfbuzz branch if you want.

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Robert Pluim <rpluim@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
>> objects in a few days, please push.
>
> I built this on GNU/Linux with the following configs:
>
> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
> ./configure --with-cairo   # This builds ftfont.o ftcrfont.o
> ./configure --without-xft  # This builds xfont.o only
>
> Pushed to emacs-26 as 
>
> Just kidding. Pushed to master as
>
> I can cherry-pick this to the harfbuzz branch if you want.

ENOCOFFEE.

Pushed to master as b0088103bc

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Robert Pluim <rpluim@gmail.com> writes:

> Robert Pluim <rpluim@gmail.com> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
>>> objects in a few days, please push.
>>
>> I built this on GNU/Linux with the following configs:
>>
>> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
>> ./configure --with-cairo   # This builds ftfont.o ftcrfont.o
>> ./configure --without-xft  # This builds xfont.o only
>>
>> Pushed to emacs-26 as 
>>
>> Just kidding. Pushed to master as
>>
>> I can cherry-pick this to the harfbuzz branch if you want.
>
> ENOCOFFEE.
>
> Pushed to master as b0088103bc

Uhm 9e0d69b5a1 , I mean.

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: ari.roponen@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 08 Feb 2019 09:01:22 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >
> > Thanks, this LGTM, and is a significant improvement, IMO.  If no one
> > objects in a few days, please push.
> 
> I built this on GNU/Linux with the following configs:
> 
> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o

I wonder why we build both xftfont.o and ftxfont.o, when we only use
one of them, if my reading of the code is correct.

> I can cherry-pick this to the harfbuzz branch if you want.

Thanks, please do.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: ari.roponen@gmail.com,  emacs-devel@gnu.org
>> Date: Fri, 08 Feb 2019 09:01:22 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> >
>> > Thanks, this LGTM, and is a significant improvement, IMO.  If no one
>> > objects in a few days, please push.
>> 
>> I built this on GNU/Linux with the following configs:
>> 
>> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
>
> I wonder why we build both xftfont.o and ftxfont.o, when we only use
> one of them, if my reading of the code is correct.
>

I did suggest removing ftxfont.o, but you said we had to deprecate it
first. Itʼs easy enough to not build in the HAVE_XFT config.

>> I can cherry-pick this to the harfbuzz branch if you want.
>
> Thanks, please do.

Will do.

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 08 Feb 2019 12:46:32 +0100
> 
> >> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
> >
> > I wonder why we build both xftfont.o and ftxfont.o, when we only use
> > one of them, if my reading of the code is correct.
> >
> 
> I did suggest removing ftxfont.o, but you said we had to deprecate it
> first. Itʼs easy enough to not build in the HAVE_XFT config.

That's orthogonal.  Regardless of whether we want to drop ftxfont.c, I
don't understand why build with both xftfont.c and ftxfont.c.  They
seem to be mutually exclusive, from the end-user POV.

> >> I can cherry-pick this to the harfbuzz branch if you want.
> >
> > Thanks, please do.
> 
> Will do.

TIA

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Fri, 08 Feb 2019 12:46:32 +0100
>> 
>> >> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
>> >
>> > I wonder why we build both xftfont.o and ftxfont.o, when we only use
>> > one of them, if my reading of the code is correct.
>> >
>> 
>> I did suggest removing ftxfont.o, but you said we had to deprecate it
>> first. Itʼs easy enough to not build in the HAVE_XFT config.
>
> That's orthogonal.  Regardless of whether we want to drop ftxfont.c, I
> don't understand why build with both xftfont.c and ftxfont.c.  They
> seem to be mutually exclusive, from the end-user POV.
>

Removing ftxfont.c from the HAVE_XFT build is the same as dropping it:

configure.ac:

    ## We used to allow building with FreeType and without Xft.
    ## However, the ftx font backend driver is not in good shape.
    if test "$HAVE_XFT" != "yes"; then
       dnl For the "Does Emacs use" message at the end.
       HAVE_XFT=no
       HAVE_FREETYPE=no
    else
    
  elif test "$HAVE_XFT" = "yes"; then
    FONT_OBJ="$FONT_OBJ ftfont.o xftfont.o ftxfont.o"
  elif test "$HAVE_FREETYPE" = "yes"; then
    FONT_OBJ="$FONT_OBJ ftfont.o ftxfont.o"
  fi

I donʼt mind either way: as you rightly point out, nobody can possibly
be using it :-)

>> >> I can cherry-pick this to the harfbuzz branch if you want.
>> >
>> > Thanks, please do.
>> 
>> Will do.
>
> TIA

Done as 015a6e1df2

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 08 Feb 2019 15:38:52 +0100
> 
> > That's orthogonal.  Regardless of whether we want to drop ftxfont.c, I
> > don't understand why build with both xftfont.c and ftxfont.c.  They
> > seem to be mutually exclusive, from the end-user POV.
> 
> Removing ftxfont.c from the HAVE_XFT build is the same as dropping it:
> 
> configure.ac:
> 
>     ## We used to allow building with FreeType and without Xft.
>     ## However, the ftx font backend driver is not in good shape.
>     if test "$HAVE_XFT" != "yes"; then
>        dnl For the "Does Emacs use" message at the end.
>        HAVE_XFT=no
>        HAVE_FREETYPE=no
>     else
>     
>   elif test "$HAVE_XFT" = "yes"; then
>     FONT_OBJ="$FONT_OBJ ftfont.o xftfont.o ftxfont.o"
>   elif test "$HAVE_FREETYPE" = "yes"; then
>     FONT_OBJ="$FONT_OBJ ftfont.o ftxfont.o"
>   fi
> 
> I donʼt mind either way: as you rightly point out, nobody can possibly
> be using it :-)

The above is strange, since the code which uses font backends clearly
meant something else:

  #ifdef USE_CAIRO
    register_font_driver (&ftcrfont_driver, f);
  #else
  #ifdef HAVE_FREETYPE
  #ifdef HAVE_XFT
    register_font_driver (&xftfont_driver, f);
  #else	/* not HAVE_XFT */
    register_font_driver (&ftxfont_driver, f);
  #endif	/* not HAVE_XFT */
  #endif	/* HAVE_FREETYPE */
    register_font_driver (&xfont_driver, f);
  #endif	/* not USE_CAIRO */

Here, xftfont and ftxfont are clearly 2 more-or-less equivalent
alternatives, and bot rely on Freetype.  So I don't understand why not
having XFT is taken by configure.ac to mean there's no Freetype,
either.  Sounds like a mistake, or did I miss something?

> >> >> I can cherry-pick this to the harfbuzz branch if you want.
> >> >
> >> > Thanks, please do.
> >> 
> >> Will do.
> >
> > TIA
> 
> Done as 015a6e1df2

Thanks.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

> The above is strange, since the code which uses font backends clearly
> meant something else:
>
>   #ifdef USE_CAIRO
>     register_font_driver (&ftcrfont_driver, f);
>   #else
>   #ifdef HAVE_FREETYPE
>   #ifdef HAVE_XFT
>     register_font_driver (&xftfont_driver, f);
>   #else	/* not HAVE_XFT */
>     register_font_driver (&ftxfont_driver, f);
>   #endif	/* not HAVE_XFT */
>   #endif	/* HAVE_FREETYPE */
>     register_font_driver (&xfont_driver, f);
>   #endif	/* not USE_CAIRO */
>
> Here, xftfont and ftxfont are clearly 2 more-or-less equivalent
> alternatives, and bot rely on Freetype.  So I don't understand why not
> having XFT is taken by configure.ac to mean there's no Freetype,
> either.  Sounds like a mistake, or did I miss something?
>

Interesting. Glenn made that change in
46dcfee46cb241a0f8e34da679ca8b42e8ee8d46 , but thatʼs quite a while
ago.

Like I said, we can change stuff around here, but I think the easiest
thing to do is just get rid of it.

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 08 Feb 2019 17:41:02 +0100
> 
> >   #ifdef HAVE_FREETYPE
> >   #ifdef HAVE_XFT
> >     register_font_driver (&xftfont_driver, f);
> >   #else	/* not HAVE_XFT */
> >     register_font_driver (&ftxfont_driver, f);
> >   #endif	/* not HAVE_XFT */
> >   #endif	/* HAVE_FREETYPE */
> >     register_font_driver (&xfont_driver, f);
> >   #endif	/* not USE_CAIRO */
> >
> > Here, xftfont and ftxfont are clearly 2 more-or-less equivalent
> > alternatives, and bot rely on Freetype.  So I don't understand why not
> > having XFT is taken by configure.ac to mean there's no Freetype,
> > either.  Sounds like a mistake, or did I miss something?
> 
> Interesting. Glenn made that change in
> 46dcfee46cb241a0f8e34da679ca8b42e8ee8d46 , but thatʼs quite a while
> ago.
> 
> Like I said, we can change stuff around here, but I think the easiest
> thing to do is just get rid of it.

Is XFT sufficiently widespread these days that we can rely on it being
available on most systems?  If so, I guess it's okay to drop
ftxfont.c.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Glenn Morris]

Eli Zaretskii wrote:

> Is XFT sufficiently widespread these days that we can rely on it being
> available on most systems?  If so, I guess it's okay to drop
> ftxfont.c.

Every GNU/Linux distribution provides Xft. Emacs's configure
requirements in terms of version numbers are trivially satisfied even on
the likely oldest supported distribution, RHEL6.

In recent years, there are a few (<= 5) bug reports each year from
builds that are identifiably X11 without XFT. Last year there were two.
Both were Gentoo builds passing --without-xft. One (30203) is an open
report about how the non-XFT build is worse than the XFT build.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Sat, 09 Feb 2019 14:20:13 -0500
> Cc: Robert Pluim <rpluim@gmail.com>, emacs-devel@gnu.org
> 
> Eli Zaretskii wrote:
> 
> > Is XFT sufficiently widespread these days that we can rely on it being
> > available on most systems?  If so, I guess it's okay to drop
> > ftxfont.c.
> 
> Every GNU/Linux distribution provides Xft.

Thanks.  What about non-GNU Posix hosts that run X?  Do we have any
information about the situation there?

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Stefan Monnier]

>> Every GNU/Linux distribution provides Xft.
> Thanks.  What about non-GNU Posix hosts that run X?

AFAIK the issue isn't GNU-vs-other but old-X vs new-X.  Xft is basically
part of what is expected of a recent enough X11 installation.  We could
drop ftx support from `master`s `configure` and see if anyone notices.


        Stefan

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 10 Feb 2019 09:47:38 -0500
> 
> >> Every GNU/Linux distribution provides Xft.
> > Thanks.  What about non-GNU Posix hosts that run X?
> 
> AFAIK the issue isn't GNU-vs-other but old-X vs new-X.  Xft is basically
> part of what is expected of a recent enough X11 installation.  We could
> drop ftx support from `master`s `configure` and see if anyone notices.

We need to disable it by default first.  Then, if no one complains, we
could remove it after some time.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Stefan Monnier]

>> >> Every GNU/Linux distribution provides Xft.
>> > Thanks.  What about non-GNU Posix hosts that run X?
>> AFAIK the issue isn't GNU-vs-other but old-X vs new-X.  Xft is basically
>> part of what is expected of a recent enough X11 installation.  We could
>> drop ftx support from `master`s `configure` and see if anyone notices.
> We need to disable it by default first.

If we do it with a trivial change that we can easily revert before we
start pretesting Emacs-27, I see no reason why we'd first need to change
the default.


        Stefan

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Sun, 10 Feb 2019 13:18:25 -0500
> 
> >> >> Every GNU/Linux distribution provides Xft.
> >> > Thanks.  What about non-GNU Posix hosts that run X?
> >> AFAIK the issue isn't GNU-vs-other but old-X vs new-X.  Xft is basically
> >> part of what is expected of a recent enough X11 installation.  We could
> >> drop ftx support from `master`s `configure` and see if anyone notices.
> > We need to disable it by default first.
> 
> If we do it with a trivial change that we can easily revert before we
> start pretesting Emacs-27, I see no reason why we'd first need to change
> the default.

Experience shows that the complaints come in too late, and the hope
that we can easily revert such a change is frequently thwarted.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Stefan Monnier]

> Experience shows that the complaints come in too late, and the hope
> that we can easily revert such a change is frequently thwarted.

AFAIK it's been "impossible" to compile Emacs with ftx backend but
without the xft library since 2009.

So, maybe I should rather argue that it's high time we scrap the
code altogether since it's been useless and unused for 10 years now.


        Stefan

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Experience shows that the complaints come in too late, and the hope
>> that we can easily revert such a change is frequently thwarted.
>
> AFAIK it's been "impossible" to compile Emacs with ftx backend but
> without the xft library since 2009.
>
> So, maybe I should rather argue that it's high time we scrap the
> code altogether since it's been useless and unused for 10 years now.

+1

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Glenn Morris]

Eli Zaretskii wrote:

> What about non-GNU Posix hosts that run X? 

BSDs?
The data suggest very low Emacs usage there, eg
https://debbugs.gnu.org/stats/emacs.html

Anyway, I'm sure they all have Xft too. Eg
https://svnweb.freebsd.org/ports/head/x11-fonts/libXft/
ftp://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/x11/libXft/README.html
https://man.openbsd.org/Xft.3

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: Robert Pluim <rpluim@gmail.com>,  emacs-devel@gnu.org
> Date: Sat, 09 Feb 2019 14:20:13 -0500
> 
> In recent years, there are a few (<= 5) bug reports each year from
> builds that are identifiably X11 without XFT. Last year there were two.
> Both were Gentoo builds passing --without-xft. One (30203) is an open
> report about how the non-XFT build is worse than the XFT build.

Btw, as long as we are talking about this: could you please tell why
configure.ac disables HAVE_FREETYPE when the build is without XFT?
AFAIU, ftxfont needs Freetype, so if we disabvle the latter, ftxfont
cannot be a replacement for XFT.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Glenn Morris]

Eli Zaretskii wrote:

> Btw, as long as we are talking about this: could you please tell why
> configure.ac disables HAVE_FREETYPE when the build is without XFT?
> AFAIU, ftxfont needs Freetype, so if we disabvle the latter, ftxfont
> cannot be a replacement for XFT.

I don't think it's my doing. See af17b98 and 7bbec45 (from 2009).
"We used to allow building with FreeType and without Xft.
However, the ftx font backend driver is not in good shape."
I don't imagine it has improved in the past decade.

Ref also
http://lists.gnu.org/r/emacs-devel/2009-04/msg00514.html

where pretty much the same discussion seems to have been going on.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: rpluim@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 10 Feb 2019 13:36:39 -0500
> 
> Eli Zaretskii wrote:
> 
> > Btw, as long as we are talking about this: could you please tell why
> > configure.ac disables HAVE_FREETYPE when the build is without XFT?
> > AFAIU, ftxfont needs Freetype, so if we disabvle the latter, ftxfont
> > cannot be a replacement for XFT.
> 
> I don't think it's my doing. See af17b98 and 7bbec45 (from 2009).
> "We used to allow building with FreeType and without Xft.
> However, the ftx font backend driver is not in good shape."
> I don't imagine it has improved in the past decade.
> 
> Ref also
> http://lists.gnu.org/r/emacs-devel/2009-04/msg00514.html
> 
> where pretty much the same discussion seems to have been going on.

Ah, okay.  So you are saying that we've already effectively deprecated
and disabled ftxfont 10 years ago?  In that case, we could indeed
retire it in Emacs 27.

Thanks.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Paul Eggert]

Thanks, looks good to me too. One minor point:

On 12/17/18 4:41 AM, Robert Pluim wrote:
> +#ifdef HAVE_XFT
> +#include <X11/Xlib.h>
> +#include <X11/Xft/Xft.h>
> +#endif
> +#ifdef USE_CAIRO
> +#include <cairo-ft.h>
> +#endif

Please indent the include directives by using "# include" (with a space 
after "#").

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Thanks, looks good to me too. One minor point:
>
> On 12/17/18 4:41 AM, Robert Pluim wrote:
>> +#ifdef HAVE_XFT
>> +#include <X11/Xlib.h>
>> +#include <X11/Xft/Xft.h>
>> +#endif
>> +#ifdef USE_CAIRO
>> +#include <cairo-ft.h>
>> +#endif
>
> Please indent the include directives by using "# include" (with a
> space after "#").

Fixed.

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Robert Pluim]

Robert Pluim <rpluim@gmail.com> writes:

> Paul Eggert <eggert@cs.ucla.edu> writes:
>
>> Thanks, looks good to me too. One minor point:
>>
>> On 12/17/18 4:41 AM, Robert Pluim wrote:
>>> +#ifdef HAVE_XFT
>>> +#include <X11/Xlib.h>
>>> +#include <X11/Xft/Xft.h>
>>> +#endif
>>> +#ifdef USE_CAIRO
>>> +#include <cairo-ft.h>
>>> +#endif
>>
>> Please indent the include directives by using "# include" (with a
>> space after "#").
>
> Fixed.

BTW, Emacs has ~500 instances of this rule not being followed. Iʼm
assuming that a mass change (similarly to mass whitespace fixes) would
be frowned upon.

Robert

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Wed, 19 Dec 2018 09:32:52 +0100
> 
> >> On 12/17/18 4:41 AM, Robert Pluim wrote:
> >>> +#ifdef HAVE_XFT
> >>> +#include <X11/Xlib.h>
> >>> +#include <X11/Xft/Xft.h>
> >>> +#endif
> >>> +#ifdef USE_CAIRO
> >>> +#include <cairo-ft.h>
> >>> +#endif
> >>
> >> Please indent the include directives by using "# include" (with a
> >> space after "#").
> >
> > Fixed.
> 
> BTW, Emacs has ~500 instances of this rule not being followed. Iʼm
> assuming that a mass change (similarly to mass whitespace fixes) would
> be frowned upon.

We usually fix those as part of other changes, not as a changeset in
itself.

Thanks.

Re: harfbuzz 2f72162: Fix crash in the Cairo build

[Stefan Monnier]

> [1] How many different font backends can we come up with using only 3
>      letters?

Are we limited to ASCII letters?


        Stefan