Code review comment for lp:~brandontschaefer/unity/support-Og-and-add-O2-compiler-option

Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

There are some issues here (mainly because the existing CMake stuff in Unity is not polished).

You should not overwrite the ???_CXX_FLAGS (yes, debug mode does that, and it really should not): http://voices.canonical.com/jussi.pakkanen/2013/03/26/a-list-of-common-cmake-antipatterns/

Release target type already has -O3 why would you want to replace it with -O2? Note that release mode is not used in package builds, they set their build flags manually.

Build type should be lower case or, at the very least, should work with both upper and lower case.

Does -Og work reliably enough to be enabled by default? If it may lead to "value optimized out" notifications then we might consider putting it behind an option.

review: Needs Fixing

« Back to merge proposal