[Mesa-dev] configure.ac: enable non-gallium assertions for people not using --enable-debug

Submitted by Marek Olšák on April 11, 2015, 7:11 p.m.

Details

Message ID 1428779501-27030-1-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák April 11, 2015, 7:11 p.m.
From: Marek Olšák <marek.olsak@amd.com>

---
 configure.ac | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index 4ed4b74..113fb49 100644
--- a/configure.ac
+++ b/configure.ac
@@ -421,7 +421,9 @@  if test "x$enable_debug" = xyes; then
         fi
     fi
 else
-   DEFINES="$DEFINES -DNDEBUG"
+   if [[ $DEFINES != *"-DDEBUG"* ]]; then
+      DEFINES="$DEFINES -DNDEBUG"
+   fi
 fi
 
 dnl

Comments

On Sat, Apr 11, 2015 at 12:11 PM, Marek Olšák <maraeo@gmail.com> wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> ---
>  configure.ac | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 4ed4b74..113fb49 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -421,7 +421,9 @@ if test "x$enable_debug" = xyes; then
>          fi
>      fi
>  else
> -   DEFINES="$DEFINES -DNDEBUG"
> +   if [[ $DEFINES != *"-DDEBUG"* ]]; then
> +      DEFINES="$DEFINES -DNDEBUG"
> +   fi
>  fi
>
>  dnl
> --
> 2.1.0

I'm confused, but that might just be because we have DEBUG and NDEBUG
(can we stop using DEBUG?).

The subject basically says "enable assertions if not using
--enable-debug"... to which I ask why?

But that doesn't really seem to be an accurate description of the
patch. The patch seems to be adding -DNDEBUG (which disables
assertions) if we haven't added -DDEBUG to DEFINES.

So yeah, confused.
The problem is NDEBUG is defined if --enable-debug isn't used. If I
add -DDEBUG manually to CFLAGS, both DEBUG and NDEBUG will be defined.
That means Gallium assertions will be enabled (because DEBUG is
defined) and Mesa core assertion will be disabled (because NDEBUG is
defined).

I was about to suggest if we can stop using NDEBUG, but unfortunately
the standard "assert" uses it.

Marek


On Sat, Apr 11, 2015 at 9:57 PM, Matt Turner <mattst88@gmail.com> wrote:
> On Sat, Apr 11, 2015 at 12:11 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> ---
>>  configure.ac | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 4ed4b74..113fb49 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -421,7 +421,9 @@ if test "x$enable_debug" = xyes; then
>>          fi
>>      fi
>>  else
>> -   DEFINES="$DEFINES -DNDEBUG"
>> +   if [[ $DEFINES != *"-DDEBUG"* ]]; then
>> +      DEFINES="$DEFINES -DNDEBUG"
>> +   fi
>>  fi
>>
>>  dnl
>> --
>> 2.1.0
>
> I'm confused, but that might just be because we have DEBUG and NDEBUG
> (can we stop using DEBUG?).
>
> The subject basically says "enable assertions if not using
> --enable-debug"... to which I ask why?
>
> But that doesn't really seem to be an accurate description of the
> patch. The patch seems to be adding -DNDEBUG (which disables
> assertions) if we haven't added -DDEBUG to DEFINES.
>
> So yeah, confused.
The description should be "... for people not using --enable-debug and
using -DDEBUG".

Marek

On Sat, Apr 11, 2015 at 9:57 PM, Matt Turner <mattst88@gmail.com> wrote:
> On Sat, Apr 11, 2015 at 12:11 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> ---
>>  configure.ac | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 4ed4b74..113fb49 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -421,7 +421,9 @@ if test "x$enable_debug" = xyes; then
>>          fi
>>      fi
>>  else
>> -   DEFINES="$DEFINES -DNDEBUG"
>> +   if [[ $DEFINES != *"-DDEBUG"* ]]; then
>> +      DEFINES="$DEFINES -DNDEBUG"
>> +   fi
>>  fi
>>
>>  dnl
>> --
>> 2.1.0
>
> I'm confused, but that might just be because we have DEBUG and NDEBUG
> (can we stop using DEBUG?).
>
> The subject basically says "enable assertions if not using
> --enable-debug"... to which I ask why?
>
> But that doesn't really seem to be an accurate description of the
> patch. The patch seems to be adding -DNDEBUG (which disables
> assertions) if we haven't added -DDEBUG to DEFINES.
>
> So yeah, confused.
Hi Marek,

On 11 April 2015 at 20:11, Marek Olšák <maraeo@gmail.com> wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> ---
>  configure.ac | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 4ed4b74..113fb49 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -421,7 +421,9 @@ if test "x$enable_debug" = xyes; then
>          fi
>      fi
>  else
> -   DEFINES="$DEFINES -DNDEBUG"
> +   if [[ $DEFINES != *"-DDEBUG"* ]]; then
> +      DEFINES="$DEFINES -DNDEBUG"
> +   fi
Can you use test and/or case for this kind of evaluations. Iirc using
[[ ]] is considered bash-ism, which might not be as portable.


Thanks
Emil
Sure. I have just realized the patch doesn't work, because autoconf
replaces [[ ]] with [ ].

Marek

On Sun, Apr 12, 2015 at 2:48 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Hi Marek,
>
> On 11 April 2015 at 20:11, Marek Olšák <maraeo@gmail.com> wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> ---
>>  configure.ac | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 4ed4b74..113fb49 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -421,7 +421,9 @@ if test "x$enable_debug" = xyes; then
>>          fi
>>      fi
>>  else
>> -   DEFINES="$DEFINES -DNDEBUG"
>> +   if [[ $DEFINES != *"-DDEBUG"* ]]; then
>> +      DEFINES="$DEFINES -DNDEBUG"
>> +   fi
> Can you use test and/or case for this kind of evaluations. Iirc using
> [[ ]] is considered bash-ism, which might not be as portable.
>
>
> Thanks
> Emil