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

[Diego Biurrun]

On Sun, Mar 02, 2008 at 03:15:57PM -0500, Rich Felker wrote:
> 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.

OK, let's get this clarified: We do not disagree at all.

However, MPlayer is still quite a long way from this ideal, even though
it has progressed quite a bit in the right direction.

None of this applies in this case, though.  configure does not set
TARGET_OS2, so any code contained in this conditional is likely
forgotten cruft.  As we have seen enabling the code is a good way to
have this confirmed.

Also, we already have __OS2__ in several places, so adding it in one
more place will not make the situation worse, just more consistent.

Diego