Re: [Pixman] [PATCH/RFC 0/4] New option to build pixman as C++ code (--enable-enforced-cplusplus)
On Wed, 19 Dec 2012 04:09:09 +0200
Siarhei Siamashka wrote:
> On Mon, 17 Dec 2012 01:51:26 +0100
> sandm...@cs.au.dk (Søren Sandmann) wrote:
>
> > Siarhei Siamashka writes:
> >
> > > This is not intended to be immediately pushed to pixman git repository.
> > > At least not until it proves to have some real practical use. Except
> > > maybe for the 'xor'->'filler' variable rename patch, which clearly
> > > should not cause any regressions or inconveniences.
> >
> > There is a number of things in this series that look like good changes
> > regardless of moving to C++ or not. Some I noticed:
> >
> > The SIZE_MAX change was requested here:
> >
> > http://lists.freedesktop.org/archives/pixman/2012-August/002196.html
> >
> > and someone recently mailed me privately and pointed out that Solaris 9
> > doesn't have it either.
> >
> > The xor->filler change should be harmless as you point out, and 'filler'
> > probably is a better name than 'xor'.
> >
> > The uint32->pixman_format_t in pixman-glyph.c looks good.
>
> OK, I'll try to split "[PATCH 4/4] Various remaining fixes needed for
> successful compilation with C++" and re-submit a smaller patch with
> only the changes that are not C++ specific, but also useful for C.
From 9c90e4ed1b89f6a6bb289c1d2e0d96bd09cf04e2 Mon Sep 17 00:00:00 2001
From: Siarhei Siamashka
Date: Thu, 20 Dec 2012 05:00:46 +0200
Subject: [PATCH 1/2] Define SIZE_MAX if it is not provided by the standard C
headers
C++ compilers do not define SIZE_MAX. It is also not available
if the code is compiled by some C compilers:
http://lists.freedesktop.org/archives/pixman/2012-August/002196.html
---
pixman/pixman-compiler.h |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/pixman/pixman-compiler.h b/pixman/pixman-compiler.h
index a978acc..2e45dea 100644
--- a/pixman/pixman-compiler.h
+++ b/pixman/pixman-compiler.h
@@ -56,6 +56,10 @@
# define INT64_MAX (9223372036854775807)
#endif
+#ifndef SIZE_MAX
+# define SIZE_MAX ((size_t)-1)
+#endif
+
#ifndef M_PI
# define M_PI 3.14159265358979323846
--
1.7.8.6
From 0477ba7457a705b6b668ab497a9a591fd503759a Mon Sep 17 00:00:00 2001
From: Siarhei Siamashka
Date: Thu, 20 Dec 2012 05:14:39 +0200
Subject: [PATCH 2/2] Use more appropriate types and remove a magic constant
---
pixman/pixman-glyph.c |2 +-
pixman/pixman-image.c |2 +-
pixman/pixman-region.c |2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/pixman/pixman-glyph.c b/pixman/pixman-glyph.c
index 15b3f1f..6d2c8bb 100644
--- a/pixman/pixman-glyph.c
+++ b/pixman/pixman-glyph.c
@@ -508,7 +508,7 @@ add_glyphs (pixman_glyph_cache_t *cache,
uint32_t glyph_flags = 0;
pixman_composite_func_t func = NULL;
pixman_implementation_t *implementation = NULL;
-uint32_t dest_format;
+pixman_format_code_t dest_format;
uint32_t dest_flags;
pixman_box32_t dest_box;
pixman_composite_info_t info;
diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
index 6f076d5..65041b4 100644
--- a/pixman/pixman-image.c
+++ b/pixman/pixman-image.c
@@ -888,7 +888,7 @@ pixman_image_get_format (pixman_image_t *image)
if (image->type == BITS)
return image->bits.format;
-return 0;
+return PIXMAN_null;
}
uint32_t
diff --git a/pixman/pixman-region.c b/pixman/pixman-region.c
index 8955fe7..2d6f157 100644
--- a/pixman/pixman-region.c
+++ b/pixman/pixman-region.c
@@ -202,7 +202,7 @@ PIXREGION_SZOF (size_t n)
return size + sizeof(region_data_type_t);
}
-static void *
+static region_data_type_t *
alloc_data (size_t n)
{
size_t sz = PIXREGION_SZOF (n);
--
1.7.8.6
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH/RFC 0/4] New option to build pixman as C++ code (--enable-enforced-cplusplus)
On Mon, 17 Dec 2012 13:10:38 -0500 Behdad Esfahbod wrote: > On 12-12-16 06:06 PM, Siarhei Siamashka wrote: > > On Sun, 16 Dec 2012 15:34:57 -0500 > > Behdad Esfahbod wrote: > > > >> On 12-12-16 12:13 AM, Siarhei Siamashka wrote: > >>> Any comments or ideas? Hopefully not a C vs. C++ flamewar :) > >> > >> I would go as far as suggesting that C++ becomes a requirement. I did > >> that in > >> HarfBuzz and never looked back. It's possible to use many useful features > >> of > >> the language (templates, etc) without linking to libstdc++, so from the > >> user's > >> point of view there's absolutely no difference between that kind of a > >> library > >> and a C library. > > > > One problem with these useful C++ features is that making use of them, > > we are wilfully becoming the hostages of a compiler. But the real > > existing compilers are struggling even to handle inline functions > > without bugs and performance regressions: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54965 > > > > So I'm leaning to remain conservative and initially use C++ features > > only as a debugging aid. Just like OpenMP, gcc vector extensions, > > 128-bit float types and the other fancy compiler features are only > > used (or proposed to be used) in the test suite. > > You definitely have a point. However, this exact example is why I always > thought Pixman in particular can make good use of templates. Because > essentially that's what Pixman is: operation templates instantiated with lots > of different options. Yes, C++ templates are surely more convenient for this type of code. > Currently we just use always_inline and hope that the compiler > does the right thing. With always_inline we can be sure that the compiler does the right thing. When it does not, this is very visible and hard not to notice, because gcc treats inability to actually inline an always_inline function as error (that's what the bug from gcc bugzilla is about). Doing it in the compiler endorsed way would mean to just use ordinary 'inline' keyword and let the compiler decide whether to honour it or not. > With C++ those become template instantiations that have much more > chance to be inlined and optimized. It's more like the other way around. The compiler has more chances to screw up the performance when more complex features are used. It's basically a trade off between having more convenience and having more control over how the code is translated to the machine instructions in the end. The intrinsics are also a convenience feature which sacrifices some performance. But that's another topic. > Incidentally, I think that's the approach Skia took. And also Qt raster backend. -- Best regards, Siarhei Siamashka ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH/RFC 0/4] New option to build pixman as C++ code (--enable-enforced-cplusplus)
On Mon, 17 Dec 2012 01:51:26 +0100 sandm...@cs.au.dk (Søren Sandmann) wrote: > Siarhei Siamashka writes: > > > This is not intended to be immediately pushed to pixman git repository. > > At least not until it proves to have some real practical use. Except > > maybe for the 'xor'->'filler' variable rename patch, which clearly > > should not cause any regressions or inconveniences. > > There is a number of things in this series that look like good changes > regardless of moving to C++ or not. Some I noticed: > > The SIZE_MAX change was requested here: > > http://lists.freedesktop.org/archives/pixman/2012-August/002196.html > > and someone recently mailed me privately and pointed out that Solaris 9 > doesn't have it either. > > The xor->filler change should be harmless as you point out, and 'filler' > probably is a better name than 'xor'. > > The uint32->pixman_format_t in pixman-glyph.c looks good. OK, I'll try to split "[PATCH 4/4] Various remaining fixes needed for successful compilation with C++" and re-submit a smaller patch with only the changes that are not C++ specific, but also useful for C. -- Best regards, Siarhei Siamashka ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH/RFC 0/4] New option to build pixman as C++ code (--enable-enforced-cplusplus)
On 12-12-16 06:06 PM, Siarhei Siamashka wrote: > On Sun, 16 Dec 2012 15:34:57 -0500 > Behdad Esfahbod wrote: > >> On 12-12-16 12:13 AM, Siarhei Siamashka wrote: >>> Any comments or ideas? Hopefully not a C vs. C++ flamewar :) >> >> I would go as far as suggesting that C++ becomes a requirement. I did that >> in >> HarfBuzz and never looked back. It's possible to use many useful features of >> the language (templates, etc) without linking to libstdc++, so from the >> user's >> point of view there's absolutely no difference between that kind of a library >> and a C library. > > One problem with these useful C++ features is that making use of them, > we are wilfully becoming the hostages of a compiler. But the real > existing compilers are struggling even to handle inline functions > without bugs and performance regressions: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54965 > > So I'm leaning to remain conservative and initially use C++ features > only as a debugging aid. Just like OpenMP, gcc vector extensions, > 128-bit float types and the other fancy compiler features are only > used (or proposed to be used) in the test suite. You definitely have a point. However, this exact example is why I always thought Pixman in particular can make good use of templates. Because essentially that's what Pixman is: operation templates instantiated with lots of different options. Currently we just use always_inline and hope that the compiler does the right thing. With C++ those become template instantiations that have much more chance to be inlined and optimized. Incidentally, I think that's the approach Skia took. Anyway, just dropping ideas on the table. -- behdad http://behdad.org/ ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH/RFC 0/4] New option to build pixman as C++ code (--enable-enforced-cplusplus)
Siarhei Siamashka writes: > This is not intended to be immediately pushed to pixman git repository. > At least not until it proves to have some real practical use. Except > maybe for the 'xor'->'filler' variable rename patch, which clearly > should not cause any regressions or inconveniences. There is a number of things in this series that look like good changes regardless of moving to C++ or not. Some I noticed: The SIZE_MAX change was requested here: http://lists.freedesktop.org/archives/pixman/2012-August/002196.html and someone recently mailed me privately and pointed out that Solaris 9 doesn't have it either. The xor->filler change should be harmless as you point out, and 'filler' probably is a better name than 'xor'. The uint32->pixman_format_t in pixman-glyph.c looks good. Regarding the malloc() changes, it might be worthwhile to add some macros that take a typename as a parameter, along the lines of #define pixman_new(type, n)\ ((type *)pixman_malloc_ab (sizeof (type), n)) so that we get warnings if we try to assign the result to an incorrect pointer type. Although for a lot of code in pixman, we do actually rely on the ability to do type aliasing of malloc()ed memory, so maybe the usefulness of such macros is limited. > Any comments or ideas? Hopefully not a C vs. C++ flamewar :) Every once in a while I let myself be convinced me that C++ can be used as a better C, but whenever I have tried it, it has somehow always ended up driving me crazy with longer compile times and casts all over the place. I guess I could see the potential for better debug builds that you mention in the first commit message, but I'd definitely like to see some concrete benefits before committing to keep all pixman code compilable as C++. Søren ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH/RFC 0/4] New option to build pixman as C++ code (--enable-enforced-cplusplus)
On Sun, 16 Dec 2012 15:34:57 -0500 Behdad Esfahbod wrote: > On 12-12-16 12:13 AM, Siarhei Siamashka wrote: > > Any comments or ideas? Hopefully not a C vs. C++ flamewar :) > > I would go as far as suggesting that C++ becomes a requirement. I did that in > HarfBuzz and never looked back. It's possible to use many useful features of > the language (templates, etc) without linking to libstdc++, so from the user's > point of view there's absolutely no difference between that kind of a library > and a C library. One problem with these useful C++ features is that making use of them, we are wilfully becoming the hostages of a compiler. But the real existing compilers are struggling even to handle inline functions without bugs and performance regressions: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54965 So I'm leaning to remain conservative and initially use C++ features only as a debugging aid. Just like OpenMP, gcc vector extensions, 128-bit float types and the other fancy compiler features are only used (or proposed to be used) in the test suite. -- Best regards, Siarhei Siamashka ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH/RFC 0/4] New option to build pixman as C++ code (--enable-enforced-cplusplus)
On 12-12-16 12:13 AM, Siarhei Siamashka wrote: > Any comments or ideas? Hopefully not a C vs. C++ flamewar :) I would go as far as suggesting that C++ becomes a requirement. I did that in HarfBuzz and never looked back. It's possible to use many useful features of the language (templates, etc) without linking to libstdc++, so from the user's point of view there's absolutely no difference between that kind of a library and a C library. -- behdad http://behdad.org/ ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
