[MPlayer-cvslog] r26219 - trunk/mpbswap.h

[Aurelien Jacobs]

Reimar Döffinger wrote:

> On Tue, Mar 11, 2008 at 07:30:20PM +0100, Aurelien Jacobs wrote:
> > Ivan Kalvachev wrote:
> > 
> > > On Tue, Mar 11, 2008 at 1:29 PM, Reimar Döffinger
> > > <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> > > >
> > > > On Tue, Mar 11, 2008 at 01:20:31PM +0200, Ivan Kalvachev wrote:
> > > >  > On Tue, Mar 11, 2008 at 12:42 PM, Reimar Döffinger
> > > >  > <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> > > >  > > On Mon, Mar 10, 2008 at 09:20:08PM +0100, diego wrote:
> > > >  > >  > Author: diego
> > > >  > >  > Date: Mon Mar 10 21:20:08 2008
> > > >  > >  > New Revision: 26219
> > > >  > >  >
> > > >  > >  > Log:
> > > >  > >  > Add missing header #include.
> > > >  > >  >
> > > >  > >  >
> > > >  > >  > Modified:
> > > >  > >  >    trunk/mpbswap.h
> > > >  > >  >
> > > >  > >  > Modified: trunk/mpbswap.h
> > > >  > >  > ==============================================================================
> > > >  > >  > --- trunk/mpbswap.h   (original)
> > > >  > >  > +++ trunk/mpbswap.h   Mon Mar 10 21:20:08 2008
> > > >  > >  > @@ -3,6 +3,8 @@
> > > >  > >  >
> > > >  > >  >  #include <sys/types.h>
> > > >  > >  >  #include "libavutil/bswap.h"
> > > >  > >  > +#include "config.h"
> > > >  > >  > +
> > > >  > >
> > > >  > >  Uh, libavutil/bswap.h almost certainly requires config.h,
> > > >  > > this seems like a stupid place to put it?
> > 
> > Uh, libavutil/bswap.h almost certainly doesn't require anything as
> > it is supposed to be self-contained.
> 
> "Require" as in "needs this header to do its work". Sure, it does
> include it by itself, but putting config.h below IMO gives a
> completely wrong impression.

The only impression it is supposed to give is that some definitions
from config.h are required by mpbswap.h itself.
Every other impression is wrong, whatever the position of the
include is.

> And as to a way it can fail (admittedly very obscure): assume a header
> that has problems with something config.h defines. In above code
> intuitively above config.h would seem the right place to include it -
> but that would be wrong since config.h gets included by
> libavutil/bswap.h

I think that the fact that a header has problems with defines from
another header should be considered as a bug and should be fixed
(be it namespace colision or whatever).
It would cause subtle breakage at some point anyway.

Aurel