[MPlayer-cvslog] r28360 - in trunk: Makefile mencoder.c mpcommon.c mpcommon.h mplayer.c version.sh

[The Wanderer]

Diego Biurrun wrote:

> On Sun, Jan 25, 2009 at 11:54:53PM +0100, Aurelien Jacobs wrote:
 >
>> diego wrote:
>> 
>> > Log:
>> > Factorize print_version().
>> > 
>> > [...]
>> > @@ -24,6 +28,47 @@ ass_track_t* ass_track = 0; // current t
>> >  
>> > +#if HAVE_MMX
>> > +    mp_msg(MSGT_CPLAYER,MSGL_V," MMX");
>> > +#endif
>> > +#if HAVE_MMX2
>> > +    mp_msg(MSGT_CPLAYER,MSGL_V," MMX2");
>> > +#endif
>> > +#if HAVE_3DNOW
>> > +    mp_msg(MSGT_CPLAYER,MSGL_V," 3DNow");
>> > +#endif
>> > +#if HAVE_3DNOWEX
>> > +    mp_msg(MSGT_CPLAYER,MSGL_V," 3DNowEx");
>> > +#endif
>> > +#if HAVE_SSE
>> > +    mp_msg(MSGT_CPLAYER,MSGL_V," SSE");
>> > +#endif
>> > +#if HAVE_SSE2
>> > +    mp_msg(MSGT_CPLAYER,MSGL_V," SSE2");
>> > +#endif
>> > +    mp_msg(MSGT_CPLAYER,MSGL_V,"\n");
>> > +#endif /* RUNTIME_CPUDETECT */
>> > +#endif /* ARCH_X86 */
>> > +}
>> 
>> Unrelated to this commit, but this could be written nicer, smaller,
>> more readable:
>> 
>> if (HAVE_MMX)    mp_msg(MSGT_CPLAYER,MSGL_V," MMX"  );
>> if (HAVE_MMX2)   mp_msg(MSGT_CPLAYER,MSGL_V," MMX2" );
>> if (HAVE_3DNOW)  mp_msg(MSGT_CPLAYER,MSGL_V," 3DNow");
> 
> Done, thanks for the hint.

As far as I can tell, it wasn't done, at least not as suggested.

As I read it, the important part of the suggested change was the
readability increase, which in turn derives almost entirely from moving
the mp_msg call onto the same line as the conditional and doing vertical
alignment as appropriate. In order to achieve that, it's necessary to
convert the #if X into if(X), but that change is not itself especially
important.

Commit 28375, which I think is the one corresponding to the change
you're talking about, does the #if -> if() conversion but does not
combine the lines. I don't think I see how this is an improvement.
Shouldn't the rest of the suggested change be made?

(Sorry for the very delayed response - I'm catching up from
late December, having fallen behind for reasons which would probably
seem silly to most other people.)

-- 
       The Wanderer

Warning: Simply because I argue an issue does not mean I agree with any
side of it.

Secrecy is the beginning of tyranny.