[PATCH] cmake.eclass: do not append -DNDEBUG to CPPFLAGS

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] cmake.eclass: do not append -DNDEBUG to CPPFLAGS

Mike Gilbert-2
The NDEBUG macro turns the assert() function into a noop. This gives a
small performance boost, but may allow subtle programming errors to go
unnoticed.

This code was added back in 2008, when we started passing
-DCMAKE_BUILD_TYPE=None instead of Release or Debug. It probably tries
to mimic a default behavior of Release type builds.

Other common build systems do not do this by default. For example,
autoconf's AC_HEADER_ASSERT macro only sets NDEBUG if --disable-assert
is passed to configure (it defaults to enabled).

It is better to let users add this to CPPFLAGS themselves if they really
want to save those few CPU cycles.

Signed-off-by: Mike Gilbert <[hidden email]>
---
 eclass/cmake.eclass | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/eclass/cmake.eclass b/eclass/cmake.eclass
index 160f40b1cf8e..3da3b9aeb555 100644
--- a/eclass/cmake.eclass
+++ b/eclass/cmake.eclass
@@ -371,15 +371,6 @@ cmake_src_configure() {
  # Fix xdg collision with sandbox
  xdg_environment_reset
 
- # @SEE CMAKE_BUILD_TYPE
- if [[ ${CMAKE_BUILD_TYPE} = Gentoo ]]; then
- # Handle release builds
- if ! in_iuse debug || ! use debug; then
- local CPPFLAGS=${CPPFLAGS}
- append-cppflags -DNDEBUG
- fi
- fi
-
  # Prepare Gentoo override rules (set valid compiler, append CPPFLAGS etc.)
  local build_rules=${BUILD_DIR}/gentoo_rules.cmake
 
--
2.26.0.rc2


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] cmake.eclass: do not append -DNDEBUG to CPPFLAGS

David Seifert
On Sun, 2020-03-29 at 21:03 -0400, Mike Gilbert wrote:

> The NDEBUG macro turns the assert() function into a noop. This gives a
> small performance boost, but may allow subtle programming errors to go
> unnoticed.
>
> This code was added back in 2008, when we started passing
> -DCMAKE_BUILD_TYPE=None instead of Release or Debug. It probably tries
> to mimic a default behavior of Release type builds.
>
> Other common build systems do not do this by default. For example,
> autoconf's AC_HEADER_ASSERT macro only sets NDEBUG if --disable-assert
> is passed to configure (it defaults to enabled).
>
> It is better to let users add this to CPPFLAGS themselves if they really
> want to save those few CPU cycles.
>
> Signed-off-by: Mike Gilbert <[hidden email]>
> ---
>  eclass/cmake.eclass | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/eclass/cmake.eclass b/eclass/cmake.eclass
> index 160f40b1cf8e..3da3b9aeb555 100644
> --- a/eclass/cmake.eclass
> +++ b/eclass/cmake.eclass
> @@ -371,15 +371,6 @@ cmake_src_configure() {
>   # Fix xdg collision with sandbox
>   xdg_environment_reset
>  
> - # @SEE CMAKE_BUILD_TYPE
> - if [[ ${CMAKE_BUILD_TYPE} = Gentoo ]]; then
> - # Handle release builds
> - if ! in_iuse debug || ! use debug; then
> - local CPPFLAGS=${CPPFLAGS}
> - append-cppflags -DNDEBUG
> - fi
> - fi
> -
>   # Prepare Gentoo override rules (set valid compiler, append CPPFLAGS
> etc.)
>   local build_rules=${BUILD_DIR}/gentoo_rules.cmake
>  

Double ACK. Injecting this flag is imo doing too much and more feature creep
than anything else. This is why we should try and find ways of checking that
CPPFLAGS is respected as a user flag.