[MPlayer-dev-eng] [PATCH] Support compiling without CPU optimization

[Reimar Döffinger]

Note that I didn't realize it was committed, so most of my original
comments don't really matter.
I'd only request a different name or at least more verbose
description, because I really think it is completely misleading.

On Thu, Jun 04, 2015 at 01:45:11AM +0200, Ingo Brückl wrote:
> Reimar Döffinger wrote on Wed, 3 Jun 2015 19:02:11 +0200:
> > On Thu, May 28, 2015 at 11:04:13PM +0200, Ingo Brückl wrote:
> >> Those who have configured their gcc to already compile CPU specific,
> 
> > In what way?
> 
> Gcc configuration? --with-arch, --with-tune.
> 
> > And how then do these options break it?
> 
> Despite what the gcc docs say, -mtune=native isn't (always?) the best option
> to use and may disable features the CPU has. The kernel's Makefile, for
> example, does better by specifying -march=<cpu-type> -mtune=generic.

How is that not a gcc bug? Especially -mtune=generic being better
seems totally broken unless you actually run on an 80386.

> > I think that name is misleading
> 
> Since configure names it "GCC & CPU optimization" it seemed obvious to name
> it that way, but I'm happy with any other name as well.

That name is kind of bad as well. Removing the & would probably
make it much better.
Either way, the option is called --disable-optimization, not
--disable-gcc-cpu-optimization.
The former I'd say is not at all what it does, I would even say it doesn't disable
any optimizations at all.
FFmpeg also has a --disable-optimizations flag, and it also does
something completely different.
Our option seems to mostly disable -march, and if that's all it does that
--disable-march would be an obvious name, with "do not add -march and
-mcpu to gcc options" as description for example.
Unless of course you had further plans for this option to make it do more.

> > For example the proc=none setting is somewhat questionable
> 
> That's just an internal variable, so who cares about its value? Well, the
> user does, because they are presented the content as the result of the
> "optimization". (And "none" for a not recognized CPU isn't probably worse
> than "error" - even internally).

I would claim "error" is clearer as it at least indicates an
unusual/unexpected special case, while "none" could mean anything.
But my complaint was mostly based on me not being able to understand
what the patch does, which I now figured had a different reason:
out of the 29 added/changed lines, 10 were due to what I'd
consider "cosmetics" - changing "error" to "none" and
"GCC & CPU optimization abilities" to "GCC & CPU optimization".
I'd kindly request that to be separated in the future :)

> > If it's only -march and -mcpu it seems preferable to filter out,
> > probably it would be easiest to just leave that up to everyone via
> > a sed command or otherwise in the cflag setting code.
> 
> Since the default doesn't change the former behaviour and the new option
> doesn't break anything, I don't see the problem. What is the benefit of
> "leaving that up to everyone via a sed command or otherwise in the cflag
> setting code" vs. a simple configure option?

Mostly the fact that to me it looked(looks?) like an option that is only useful
for one person (and in addition a gcc bug workaround IMHO) that everyone
else trying to use has to read and possibly understand.
So far, seen over the overall user-base the cost/benefit doesn't seem
very convincing.
Now that I've actually understood the patch though I don't really mind
as long as (as said) the name and description is improved.