[MPlayer-cvslog] r26111 - trunk/mp_msg.h

[Rich Felker]

On Sun, Mar 02, 2008 at 07:32:57PM +0100, Diego Biurrun wrote:
> On Sat, Mar 01, 2008 at 11:35:08PM -0500, Rich Felker wrote:
> > On Sat, Mar 01, 2008 at 11:38:23AM +0100, Diego Biurrun wrote:
> > > On Thu, Feb 28, 2008 at 10:53:06PM -0500, Rich Felker wrote:
> > > > On Thu, Feb 28, 2008 at 07:31:35PM +0100, diego wrote:
> > > > > 
> > > > > Log:
> > > > > TARGET_OS2 is never set, use __OS2__ instead.
> > > > > 
> > > > > --- trunk/mp_msg.h	(original)
> > > > > +++ trunk/mp_msg.h	Thu Feb 28 19:31:35 2008
> > > > > @@ -111,7 +111,7 @@ int mp_msg_test(int mod, int lev);
> > > > >  
> > > > > -#ifdef TARGET_OS2
> > > > > +#ifdef __OS2__
> > > > 
> > > > Again, I'm against this general class of change. I've explained the
> > > > reasoning many times! Would you please honor that or at least discuss
> > > > it rather than continuing these bad commits?
> > > 
> > > I've discussed it before, I get tired of repeating it: I have little
> > > interest in discussing your theories.  There is no practical alternative
> > > to what I committed.
> > > 
> > > If you disagree: Patches welcome.
> > 
> > I could write the code to detect OS2 and add #define TARGET_OS2 in
> > configure, but I figure you're more qualified as the configure script
> > maintainer and someone familiar with it... While you're fixing the
> > problem described in this commit, you might as well fix it right..
> 
> I don't see what we would gain by using TARGET_OS2 instead of __OS2__.

What we gain is that we don't suffer from bitrot. That is, if someone
tries to use an old MPlayer, or if MPlayer eventually goes
unmaintained (or at least the code specific to some obscure OS like
OS2 is unmaintained) for a long period, someone trying to fix it
doesn't have to hunt down all the implementation-specific assumptions
(which may no longer be correct) in preprocessor conditionals all over
the source. Instead they just need to adapt a few lines in configure.

I've seen plenty of this crap in other code that's gone unmaintained
(or poorly maintained) and fixing it retroactively is a nightmare.
This is why I strongly object to writing such ugliness in the first
place. Policy for preprocessor conditionals should be:

1. Preprocessor conditionals must always be based on a condition
determined by configure, not symbols defined by the particular
compiler, kernel headers, libc headers, etc. in use at the time the
code was written.

2. Preprocessor symbols defined by configure for conditional
compilation must always indicate the presence or absence of a
particular feature/API, and must not reflect assumptions that a
particular compiler/OS/kernel/etc. does (or does not) have certain
properties.

Examples:

- Inline asm should not be dependent on gcc version but instead on
  ability to compile asm and suitability of that asm for the target
  machine.

- Use of OSS sound (or locations of the headers) should not be
  dependent on the detected "OS name" but instead on tests for the
  headers. Maybe someday (sadly...I sure hope not) Linux will remove
  OSS API support, etc.

- WinAPI stuff should not be dependent on OS being Windows, because as
  we've seen, OS/2 has some of the same features.

etc.etc.etc.

> Also, if we would want to start going down that route, we have a hell of
> a lot of stuff to fix.

Most of which was added over my objection..

> Anyway, the code we are talking about here is gone now.

:)

Rich