[Bug 2455] Review request: pcsx2 - A Sony Playstation2 emulator

[RPM Fusion Bugzilla]

https://bugzilla.rpmfusion.org/show_bug.cgi?id=2455

--- Comment #55 from Andrea Musuruane <musuruan at gmail.com> 2013-06-27 11:00:03 CEST ---
The backslash there is at the end of the last line of the %cmake command is
wrong - you are safe only because there is an empty line next.

(In reply to comment #54)
> Okay that should be fixed. rpmlint doesn't like the Source http, but the link
> works fine when I try it.

Ignore the warning about the Source URL. Spectool -g can download it fine.

> As far as the bug report about patch to upstream goes, could you please confirm
> that this is correct that the -m32 CFLAG needs to be unset, as they respond
> "Without this flag it won't build anymore on 64 bits machine."
> 
> What is the issue exactly for Fedora? and perhaps explain why this is so? I can
> then add this to upstream bug report.

These are the CFLAGS used by Fedora under i686:
"-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -grecord-gcc-switches  -m32 -march=i686 -mtune=atom
-fasynchronous-unwind-tables"

Pcsx2 can only run under i686. So you can safely drop your patch.

"Compilers used to build packages must honor these flags. Honoring means that
the contents of that variable is used as the basis of the flags actually used
by the compiler during the package build." (from Fedora guidelines)

And you are using them as a base. This is fine.

"Overriding these flags for performance optimizations (for instance, -O3
instead of -O2) is generally discouraged. "

Pcsx2 uses many optimization flags (e.g. -fxxxxx). 

Please also notice that -O2 already turns on a number of optimizations:
http://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Optimize-Options.html

As you can see most flags are already turned on by -O2.

I cannot find any reference in the gcc documentation of recent versions of
"-ftree-lrs", "-fvisibility=hidden". I presumed they been dropped.

Moreover there are these:
-fno-guess-branch-probability -fno-dse -fno-strict-aliasing -fno-tree-dse

These are in contrast with optimizations turned on by "-O2".

The guidelines says:
"If you can present benchmarks that show a significant speedup for this
particular code, this could be revisited on a case-by-case basis. Adding to and
overriding or filtering parts of these flags is permitted if there's a good
reason to do so; the rationale for doing so must be documented in the specfile.
"

Therefore to follow the guidelines, you should ask upstream about the 4 above
flags and why are they used. If I was upstream, I'd clean up them a bit since
most are already triggered by "-O2".

The rest of the spec file seems fine to me. Good work!

-- 
Configure bugmail: https://bugzilla.rpmfusion.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.