Re: [Patches] Portability, support for CloudABI, etc.

Hi Moritz,

2015-11-21 11:39 GMT+01:00 Moritz Bunkus <[email protected]>:
> 1. The new constructor taking a FILE*
>
> You've unconditionally added a new constructor
> StdIOCallback(FILE*). While not breaking the API it does break the
> ABI. I've taken the liberty to modify your patch to only define that
> constructor if fopen() is NOT available.

I have to confess I'm not a C++ ABI expert, but how does adding an
additional constructor break the ABI? Adding additional virtual
functions will break its vtable, but as far as I know, adding an extra
constructor would do nothing in addition to exporting another symbol,
right?

> 2. Including ebml_config.h
>
> This is probably not that easy to solve, or I simply lack the experience
> creating libraries that include their autoconf'd config.h file somehow.
>
> I've tried compiling MKVToolNix against a libEBML that includes the v2
> patch. This spits out a lot of warnings:
>
> In file included from src/common/make_unique.h:18:0,
>                  from src/common/common.h:91,
>                  from src/common/common_pch.h:4,
>                  from src/common/clpi.cpp:14:
> ./config.h:167:0: warning: "PACKAGE_TARNAME" redefined
>  #define PACKAGE_TARNAME "mkvtoolnix"
>  ^
> ...
>
> This is not acceptable, but totally understandable, and all programs
> using autoconf and libEBML will run into this.

Ah, yes. I didn't think about that. config.h defines all sorts of
random things that clutter up the global namespace, of course.

> What I think the right solution could look like is having two config.h
> files, a private and a public one. The private one is the config.h
> that's already being created by libEBML's autoconf. It should only be
> included by libEBML's .cpp files, not by its headers. This file wouldn't
> have to be installed/distributed either.
>
> The public include file (let's call it ebml_config.h) on the other hand
> would have to be installed/distributed as it would be included by
> libEBML's other header files. In order to minimize namespace pollution
> only the absolute minimum set of pre-processor #defines should be part
> of that file. Ideally these #defines would even use a specific prefix,
> e.g. LIBEBML_HAVE_FOPEN.
>
> I don't know if that's possible (or easy to do) with autoconf,
> though. You seem to have (much?) more knowledge about autoconf/automake,
> so maybe you have an idea how to implement this or even for something
> better.

This is interesting. Such a feature was already requested back in 1999:

https://www.sourceware.org/ml/autoconf/1999-11/msg00047.html

But it looks as if nothing like that was ever added. Packages like
GNOME's glib still generate their glibconfig.h header file manually
(~200 lines of m4/shell scripting).

To be honest, I don't think it's worth investing that amount of effort
in this case. We coulkd consider just adding the following to the
existing EbmlConfig.h:

#ifndef __CloudABI__
#define LIBEBML_HAVE_FOPEN
#endif

Alternatively, I wouldn't mind holding on to the patch myself for the
time being. Adding the FILE*-based constructor would be nice, though.

-- 
Ed Schouten <[email protected]>
Nuxi, 's-Hertogenbosch, the Netherlands
KvK-nr.: 62051717
_______________________________________________
Matroska-devel mailing list
[email protected]
http://lists.matroska.org/cgi-bin/mailman/listinfo/matroska-devel
Read Matroska-Devel on GMane: http://dir.gmane.org/gmane.comp.multimedia.matroska.devel