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

https://bugzilla.rpmfusion.org/show_bug.cgi?id=2455 --- Comment #55 from Andrea Musuruane <musuruan(a)gmail.com&gt; 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) Ignore the warning about the Source URL. Spectool -g can download it fine. 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.