Re: Stack module and -Wsign-compare
Thank you, Paul. My local tests also show that it behaves better now. Am Mi., 5. Jan. 2022 um 20:54 Uhr schrieb Paul Eggert : > > On 1/5/22 10:00, Bruno Haible wrote: > > Another possible fix would be to change > >size_t size; > > to > >idx_t size; > > in the struct. > > Yes, that fits better into our strategy of preferring signed to unsigned > types for sizes. Plus, it avoids a cast that's too powerful in C. I > installed the attached.
Re: Stack module and -Wsign-compare
On 1/5/22 10:00, Bruno Haible wrote:
Another possible fix would be to change
size_t size;
to
idx_t size;
in the struct.
Yes, that fits better into our strategy of preferring signed to unsigned
types for sizes. Plus, it avoids a cast that's too powerful in C. I
installed the attached.From 403968da56573bdbdcab79adabddcdc270e05cc9 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 5 Jan 2022 11:37:26 -0800
Subject: [PATCH 1/2] stack: pacify gcc -Wsign-compare
* lib/stack.h (_GL_STACK_TYPE): Use idx_t for size too.
Suggested by Bruno Haible in:
https://lists.gnu.org/r/bug-gnulib/2022-01/msg00035.html
---
ChangeLog | 7 +++
lib/stack.h | 2 +-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index ad29aed978..609720a451 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2022-01-05 Paul Eggert
+
+ stack: pacify gcc -Wsign-compare
+ * lib/stack.h (_GL_STACK_TYPE): Use idx_t for size too.
+ Suggested by Bruno Haible in:
+ https://lists.gnu.org/r/bug-gnulib/2022-01/msg00035.html
+
2022-01-05 Bruno Haible
Fix last commit.
diff --git a/lib/stack.h b/lib/stack.h
index 26fdbad770..b4c35535a7 100644
--- a/lib/stack.h
+++ b/lib/stack.h
@@ -77,7 +77,7 @@
typedef struct
{
GL_STACK_ELEMENT *base;
- size_t size;
+ idx_t size;
idx_t allocated;
} _GL_STACK_TYPE;
--
2.32.0
From 9d3ec33a02dccaaa930f839752304d1b8d0b903e Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Wed, 5 Jan 2022 11:51:38 -0800
Subject: [PATCH 2/2] stack: also update stack_size to return idx_t
* lib/stack.h (_GL_STACK_PREFIX (size)): Return idx_t, not size_t.
---
ChangeLog | 1 +
NEWS| 3 +++
lib/stack.h | 6 +++---
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 609720a451..4829d58e83 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,7 @@
* lib/stack.h (_GL_STACK_TYPE): Use idx_t for size too.
Suggested by Bruno Haible in:
https://lists.gnu.org/r/bug-gnulib/2022-01/msg00035.html
+ (_GL_STACK_PREFIX (size)): Return idx_t, not size_t.
2022-01-05 Bruno Haible
diff --git a/NEWS b/NEWS
index 2ce1c73014..c13a115ada 100644
--- a/NEWS
+++ b/NEWS
@@ -66,6 +66,9 @@ User visible incompatible changes
DateModules Changes
+2022-01-05 stack This module now uses idx_t instead of size_t
+for indexes and counts.
+
2021-08-27 base32 These modules now use idx_t instead of size_t
base64 for indexes and counts.
diff --git a/lib/stack.h b/lib/stack.h
index b4c35535a7..28b9d7a542 100644
--- a/lib/stack.h
+++ b/lib/stack.h
@@ -17,7 +17,7 @@
/* Written by Marc Nieper-Wißkirchen , 2020. */
/* This header file does not have include-guards as it is meant to be
- includeable multiple times as long as the necessary defines have
+ includable multiple times as long as the necessary defines have
been set up.
A stack is implemented with a homogenous array of elements in
@@ -36,7 +36,7 @@
Popping:ELEMENT element = stack_pop ();
Discarding: stack_discard ();
Top-of-stack: ELEMENT element = stack_top ();
- Size: size_t size = stack_size ();
+ Size: idx_t size = stack_size ();
Here, ELEMENT is the type to which GL_STACK_ELEMENT was defined when
this file was included.
@@ -152,7 +152,7 @@ _GL_STACK_PREFIX (top) (const _GL_STACK_TYPE *stack)
}
/* Return the currently stored number of elements in the stack. */
-GL_STACK_STORAGECLASS _GL_ATTRIBUTE_PURE size_t
+GL_STACK_STORAGECLASS _GL_ATTRIBUTE_PURE idx_t
_GL_STACK_PREFIX (size) (const _GL_STACK_TYPE *stack)
{
return stack->size;
--
2.32.0
Re: Stack module and -Wsign-compare
Marc Nieper-Wißkirchen: > If "stack.h" is included by client code that is supposed to be > compiled with "-Wsign-compare", the compiler will complain about the > comparison on line 121. Indeed: although we don't care about -Wsign-compare warnings in Gnulib code, consumers of .h files from Gnulib may well do. > The simplest fix is to replace the comparison by: > > stack->size == (size_t) stack->allocated Another possible fix would be to change size_t size; to idx_t size; in the struct. Bruno
Re: stack module
Hi Bruno, Am Sa., 10. Okt. 2020 um 16:06 Uhr schrieb Bruno Haible : > > Hi Marc, > > > please see the attached patch for the new module. > > Very nice. Looks all good, except for some nit-picking in the comments: Thanks. > > > + Introspection: ELEMENT *base = stack_base (); > > I would add a comment here: > Where ELEMENT is the type to which GL_STACK_ELEMENT was defined when > this file was included. > (It would be tempting to write > GL_STACK_ELEMENT *base = stack_base (); > but GL_STACK_ELEMENT is no longer defined after the file was included...) I will add this comment. > Other than that, please feel free to commit and push this. Thanks!! I will do this as soon as I have been approved by Paul. (Do I have to do something about the ChangeLog entry or will it be auto-generated from my Git commit message?) Marc
Re: stack module
Hi Marc, > please see the attached patch for the new module. Very nice. Looks all good, except for some nit-picking in the comments: > + Introspection: ELEMENT *base = stack_base (); I would add a comment here: Where ELEMENT is the type to which GL_STACK_ELEMENT was defined when this file was included. (It would be tempting to write GL_STACK_ELEMENT *base = stack_base (); but GL_STACK_ELEMENT is no longer defined after the file was included...) Other than that, please feel free to commit and push this. Thanks!! Bruno
Re: stack module
Dear Bruno,
please see the attached patch for the new module.
Thanks,
Marc
Am Mi., 7. Okt. 2020 um 11:01 Uhr schrieb Marc Nieper-Wißkirchen
:
>
> Hi Bruno,
>
> I am finally finishing my work on the stack module.
>
> Am Do., 23. Juli 2020 um 12:36 Uhr schrieb Bruno Haible :
>
> [...]
>
> > This is perfectly acceptable for Gnulib. It has debuggability and type
> > safety.
> >
> > You have precedent e.g. in lib/diffseq.h and lib/aligned-malloc.h.
> >
> > You can even omit the 'GL_' prefix from the macro names. The user can #undef
> > the macros after including the file; therefore there is nearly no risk of
> > collision with macros defined by other code.
>
> After I have given it a bit more thought, I see a different type of
> collision possibility, namely if some header file uses the stack
> header file. It will leak the macro names. Assume that the stack
> module accepts the macro name ELEMENT for the element type. A
> hypothetical header for the frob type may be implemented as:
>
> #ifndef FROB_H
> #define FROB_H
> ...
> #define ELEMENT int
> #define STORAGECLASS static inline
> #include "stack.h"
> ...
> struct frob
> {
> stack frob_stack;
> ...
> };
> ...
> #endif /* FROB_H */
>
> Now, user code cannot do:
>
> #define ELEMENT hydrogen
> #include "frob.h"
> ...
>
> (The ELEMENT definition in the first line could in fact be exported by
> some other header file.)
>
> Therefore, I think I prefer to use prefixed names.
>
> Cheers,
>
> Marc
From c449d60058f4a5806729ec76779fbb050890ddaf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc=20Nieper-Wi=C3=9Fkirchen?=
Date: Wed, 7 Oct 2020 13:29:10 +0200
Subject: [PATCH] Type-safe stack data type. * MODULES.html.sh: Add entry for
the stack module. * modules/stack: New file. * modules/stack-tests: New file.
* lib/stack.h: New file. * tests/test-stack.c: New file.
---
ChangeLog | 9 +++
MODULES.html.sh | 1 +
lib/stack.h | 163
modules/stack | 25 +++
modules/stack-tests | 13
tests/test-stack.c | 74
6 files changed, 285 insertions(+)
create mode 100644 lib/stack.h
create mode 100644 modules/stack
create mode 100644 modules/stack-tests
create mode 100644 tests/test-stack.c
diff --git a/ChangeLog b/ChangeLog
index 875f3551a..b64689f73 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-10-07 Marc nieper-Wißkirchen
+
+ stack: New module.
+ * MODULES.html.sh: Add entry for the stack module.
+ * modules/stack: New file.
+ * modules/stack-tests: New file.
+ * lib/stack.h: New file.
+ * tests/test-stack.c: New file.
+
2020-10-05 Paul Eggert
thread: pacify GCC on Solaris 10
diff --git a/MODULES.html.sh b/MODULES.html.sh
index a8a629e29..7e7cdae3e 100755
--- a/MODULES.html.sh
+++ b/MODULES.html.sh
@@ -2031,6 +2031,7 @@ func_all_modules ()
func_module readline
func_module readtokens
func_module readtokens0
+ func_module stack
func_module strverscmp
func_module filevercmp
func_end_table
diff --git a/lib/stack.h b/lib/stack.h
new file mode 100644
index 0..22e976952
--- /dev/null
+++ b/lib/stack.h
@@ -0,0 +1,163 @@
+/* Type-safe stack data type.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+
+ This program is free software: you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see
Re: stack module
Hi Bruno,
I am finally finishing my work on the stack module.
Am Do., 23. Juli 2020 um 12:36 Uhr schrieb Bruno Haible :
[...]
> This is perfectly acceptable for Gnulib. It has debuggability and type safety.
>
> You have precedent e.g. in lib/diffseq.h and lib/aligned-malloc.h.
>
> You can even omit the 'GL_' prefix from the macro names. The user can #undef
> the macros after including the file; therefore there is nearly no risk of
> collision with macros defined by other code.
After I have given it a bit more thought, I see a different type of
collision possibility, namely if some header file uses the stack
header file. It will leak the macro names. Assume that the stack
module accepts the macro name ELEMENT for the element type. A
hypothetical header for the frob type may be implemented as:
#ifndef FROB_H
#define FROB_H
...
#define ELEMENT int
#define STORAGECLASS static inline
#include "stack.h"
...
struct frob
{
stack frob_stack;
...
};
...
#endif /* FROB_H */
Now, user code cannot do:
#define ELEMENT hydrogen
#include "frob.h"
...
(The ELEMENT definition in the first line could in fact be exported by
some other header file.)
Therefore, I think I prefer to use prefixed names.
Cheers,
Marc
Re: stack module
Hi Bruno, > This is perfectly acceptable for Gnulib. It has debuggability and type safety. Perfect. Then I will come up with a module in this form. > You have precedent e.g. in lib/diffseq.h and lib/aligned-malloc.h. Good to know; I hadn't taken a look at these sources yet. > You can even omit the 'GL_' prefix from the macro names. The user can #undef > the macros after including the file; therefore there is nearly no risk of > collision with macros defined by other code. Or the included file #undefs the macros as in lib/diffseq.h. Speaking of prefixes: As macro names (and other names) starting with "_GL_" are not officially allowed by ISO C, I think it makes sense to think of a new convention for new sources (while old sources could later be updated incrementally). For example, one could use the prefix "GL__" (double underscore) for (future) private identifiers. Marc
Re: stack module
Hi Marc,
> The alternative with the same type safety would be a source file with
> stack code procedures meant for inclusion (without include guards).
> The source file would expect a preprocessor defines GL_STACK_NAME,
> GL_STACK_TYPE, and GL_STACK_EXTERN.
>
> The file itself would contain code like the following:
>
> #define _GL_STACK_PREFIX(name) _GL_CONCAT(GL_STACK_NAME, _GL_CONCAT(_, name))
>
> typedef struct
> {
> GL_STACK_TYPE *base;
> size_t size;
> size_t allocated;
> }
> GL_STACK_PREFIX(type);
>
> GL_STACK_EXTERN GL_STACK_PREFIX(init) (GL_STACK_PREFIX(type) *stack)
> {
> stack->base = NULL;
> stack->size = 0;
> stack->allocated = 0;
> }
>
> ...
>
> The advantage of this model is that it generalizes to other data
> structures, for which a sole (or at least simple) macro implementation
> is not possible.
This is perfectly acceptable for Gnulib. It has debuggability and type safety.
You have precedent e.g. in lib/diffseq.h and lib/aligned-malloc.h.
You can even omit the 'GL_' prefix from the macro names. The user can #undef
the macros after including the file; therefore there is nearly no risk of
collision with macros defined by other code.
Bruno
Re: stack module
Am Sa., 23. Mai 2020 um 19:19 Uhr schrieb Bruno Haible :
>
> Marc Nieper-Wißkirchen wrote:
> > > I was expecting that you write
> > >
> > > struct
> > > {
> > > void *base; ...
> > > }
> >
> > This removes type safety. The benefit of the current approach is that
> > stack types of different types are not compatible.
>
> Indeed. Yes, it's a difficult trade-off between debuggability, binary code
> size,
> and type safety...
The alternative with the same type safety would be a source file with
stack code procedures meant for inclusion (without include guards).
The source file would expect a preprocessor defines GL_STACK_NAME,
GL_STACK_TYPE, and GL_STACK_EXTERN.
The file itself would contain code like the following:
#define _GL_STACK_PREFIX(name) _GL_CONCAT(GL_STACK_NAME, _GL_CONCAT(_, name))
typedef struct
{
GL_STACK_TYPE *base;
size_t size;
size_t allocated;
}
GL_STACK_PREFIX(type);
GL_STACK_EXTERN GL_STACK_PREFIX(init) (GL_STACK_PREFIX(type) *stack)
{
stack->base = NULL;
stack->size = 0;
stack->allocated = 0;
}
...
The advantage of this model is that it generalizes to other data
structures, for which a sole (or at least simple) macro implementation
is not possible.
What do you think?
Marc
Re: stack module
So here is the updated stack module. The name "CLEAR" has been changed
to "DESTROY" as suggested by Bruno. Error checking has been included.
The macro interface remains. Although a macro interface means macros,
the macros are trivial and the type safety wins here, I think. I have
renamed STACK_BASE to STACK_CURRENT_BASE because the base may be
invalidated when items are pushed onto the stack and reallocation has
to happen.
As I haven't heard anything from the FSF yet, I wouldn't mind if you
considered the following in the public domain...
#ifndef _GL_STACK_H
#define _GL_STACK_H
#include
#include
#include "assure.h"
#include "xalloc.h"
#define STACK(type)\
struct {\
type *base;\
size_t size;\
size_t allocated;\
}
#define STACK_INIT(stack)\
do\
{\
(stack).base = NULL;\
(stack).size = 0;\
(stack).allocated = 0;\
}\
while (0)
#define STACK_DESTROY(stack)\
free ((stack).base)
#define STACK_EMPTY(stack)\
((stack).size == 0)
#define STACK_CURRENT_BASE(stack)\
((stack).base)
#define STACK_PUSH(stack, item)\
do\
{\
if ((stack).size == (stack).allocated)\
(stack).base = x2nrealloc ((stack).base, &(stack).allocated,
sizeof (item)); \
(stack).base [(stack).size++] = item;\
}\
while (0)
#define STACK_POP(stack)\
(affirm (!STACK_EMPTY (stack)), (stack).base [--(stack).size])
#define STACK_DISCARD(stack)\
(affirm (!STACK_EMPTY (stack)), (--(stack).size))
#define STACK_TOP(stack)\
(affirm (!STACK_EMPTY (stack)), (stack).base[(stack).size - 1)
#define STACK_SIZE(stack)\
((stack).size)
#endif /* _GL_STACK_H */
Am So., 24. Mai 2020 um 21:26 Uhr schrieb Bruno Haible :
>
> Paul Eggert wrote:
> > I don't want to encourage programmers to supply an E with side effects, as
> > side
> > effects are trouble here.
>
> +1
>
Re: stack module
Paul Eggert wrote: > I don't want to encourage programmers to supply an E with side effects, as > side > effects are trouble here. +1
Re: stack module
On 5/24/20 1:17 AM, Marc Nieper-Wißkirchen wrote: > You > wrote for the affirm macro that if NDEBUG is defined that the behavior > is undefined if E has side effects. That's not true as long as E does > not evaluate to false. We can *make* it true, by fiat. :-) I don't want to encourage programmers to supply an E with side effects, as side effects are trouble here. Even if side effects happen to work in the current implementation when E is true, I don't want to suggest to programmers that they'll continue to work in future implementations; we may come up with a different implementation that evaluates E twice, for example.
Re: stack module
Thank you very much, Paul! One bit of the commentary is still misleading, though, I think. You wrote for the affirm macro that if NDEBUG is defined that the behavior is undefined if E has side effects. That's not true as long as E does not evaluate to false. Marc Am So., 24. Mai 2020 um 04:14 Uhr schrieb Paul Eggert : > > On 5/23/20 3:06 PM, Bruno Haible wrote: > > How about this instead? > > Thanks, good point about the danger. Also, I forgot to include verify.h. > > I tightened up the commentary, folded in Marc's suggestion, and installed the > attached.
Re: stack module
On 5/23/20 3:06 PM, Bruno Haible wrote: > How about this instead? Thanks, good point about the danger. Also, I forgot to include verify.h. I tightened up the commentary, folded in Marc's suggestion, and installed the attached. >From 2df56dc44200074077ebace04079ac4b0a34e4fc Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sat, 23 May 2020 19:06:16 -0700 Subject: [PATCH] =?UTF-8?q?assure:=20new=20macro=20=E2=80=98affirm?= =?UTF-8?q?=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * lib/assure.h: Include verify.h. (affirm): New macro, after a suggestion by Marc Nieper-Wißkirchen in: https://lists.gnu.org/r/bug-gnulib/2020-05/msg00263.html and commentary by Bruno Haible in: https://lists.gnu.org/r/bug-gnulib/2020-05/msg00278.html * modules/assure (Depends-on:): Add verify. --- ChangeLog | 10 ++ lib/assure.h | 24 ++-- modules/assure | 1 + 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index a7101db80..c05ef910d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2020-05-23 Paul Eggert + + assure: new macro ‘affirm’ + * lib/assure.h: Include verify.h. + (affirm): New macro, after a suggestion by Marc Nieper-Wißkirchen in: + https://lists.gnu.org/r/bug-gnulib/2020-05/msg00263.html + and commentary by Bruno Haible in: + https://lists.gnu.org/r/bug-gnulib/2020-05/msg00278.html + * modules/assure (Depends-on:): Add verify. + 2020-05-23 Bruno Haible calloc-gnu: Make test work in non-flat address spaces. diff --git a/lib/assure.h b/lib/assure.h index 8ea2f6e48..09a4edfa5 100644 --- a/lib/assure.h +++ b/lib/assure.h @@ -21,12 +21,32 @@ #define _GL_ASSURE_H #include +#include "verify.h" + +/* Evaluate an assertion E that is guaranteed to be true. + If NDEBUG is not defined, abort the program if E is false. + If NDEBUG is defined, the compiler can assume E and behavior is + undefined if E is false, fails to evaluate, or has side effects. + + Unlike standard 'assert', this macro evaluates E even when NDEBUG + is defined, so as to catch typos, avoid some GCC warnings, and + improve performance when E is simple enough. + + Also see the documentation for 'assume' in verify.h. */ + +#ifdef NDEBUG +# define affirm(E) assume (E) +#else +# define affirm(E) assert (E) +#endif /* Check E's value at runtime, and report an error and abort if not. However, do nothing if NDEBUG is defined. - Unlike standard 'assert', this macro always compiles E even when NDEBUG - is defined, so as to catch typos and avoid some GCC warnings. */ + Unlike standard 'assert', this macro compiles E even when NDEBUG + is defined, so as to catch typos and avoid some GCC warnings. + Unlike 'affirm', it is OK for E to use hard-to-optimize features, + since E is not executed if NDEBUG is defined. */ #ifdef NDEBUG # define assure(E) ((void) (0 && (E))) diff --git a/modules/assure b/modules/assure index 3cfe1f874..c37191717 100644 --- a/modules/assure +++ b/modules/assure @@ -5,6 +5,7 @@ Files: lib/assure.h Depends-on: +verify configure.ac: -- 2.17.1
Re: stack module
Hi Paul, > Something like the attached? This documentation is quite misleading: +/* Check E's value at runtime, and report an error and abort if not. + However, do nothing if NDEBUG is defined; in this case behavior is because the "do nothing" is an understatement. 'assume' is a dangerous macro, because it allows the compiler to do optimizations based on putative assumptions. Since 'affirm' invokes 'assume', the documentation should emphasize this danger. How about this instead? /* States an assertion that the programmer believes to be true. When NDEBUG is not defined, this macro is like assert: it verifies the assertion at run time and aborts the program if the assertion is not true. When NDEBUG is defined, the programmer guarantees that the assertion is true, and the macro informs the compiler about it, so that the compiler may produce optimized code based on it. */ Bruno
Re: stack module
Am Sa., 23. Mai 2020 um 22:51 Uhr schrieb Paul Eggert : > > On 5/23/20 10:38 AM, Marc Nieper-Wißkirchen wrote: > > But if "affirm" is fine with you, I would love to see it in a module. > > Either in verify or assure or in a new module named affirm. > > Something like the attached? That would be a good addition to Gnulib, indeed, I think. (And may even already be used by a number of modules, which currently call abort explicitly.) I find the comments in your code very valuable. I may change only one thing. Above the definition of affirm, you write "Unlike standard 'assert', this macro always compiles E even when NDEBUG is defined ...". I think it is better to change it to "... always evaluates E even when ...". In fact, when the assumption is fulfilled, E must have been evaluated. If the assumption fails we are in the realm of undefined behavior, so we may as well claim that E has been evaluated. This is important not only so that the side effects of the evaluation of E are documented, but also to remind the user of a potentially costly evaluation.
Re: stack module
On 5/23/20 10:38 AM, Marc Nieper-Wißkirchen wrote: > But if "affirm" is fine with you, I would love to see it in a module. > Either in verify or assure or in a new module named affirm. Something like the attached? diff --git a/lib/assure.h b/lib/assure.h index 8ea2f6e48..2f1326027 100644 --- a/lib/assure.h +++ b/lib/assure.h @@ -22,11 +22,29 @@ #include +/* Check E's value at runtime, and report an error and abort if not. + However, do nothing if NDEBUG is defined; in this case behavior is + undefined if E is false, fails to evaluate, or has side effects. + + Unlike standard 'assert', this macro always compiles E even when + NDEBUG is defined, so as to catch typos, avoid some GCC warnings, + and improve performance when E is simple enough. + + Also see the documentation for 'assume' in verify.h. */ + +#ifdef NDEBUG +# define affirm(E) assume (E) +#else +# define affirm(E) assert (E) +#endif + /* Check E's value at runtime, and report an error and abort if not. However, do nothing if NDEBUG is defined. Unlike standard 'assert', this macro always compiles E even when NDEBUG - is defined, so as to catch typos and avoid some GCC warnings. */ + is defined, so as to catch typos and avoid some GCC warnings. + Unlike 'affirm', it is OK for E to use hard-to-optimize features, + since E is not executed if NDEBUG is defined. */ #ifdef NDEBUG # define assure(E) ((void) (0 && (E))) diff --git a/modules/assure b/modules/assure index 3cfe1f874..c37191717 100644 --- a/modules/assure +++ b/modules/assure @@ -5,6 +5,7 @@ Files: lib/assure.h Depends-on: +verify configure.ac:
Re: stack module
Hi Paul, Am Sa., 23. Mai 2020 um 19:33 Uhr schrieb Paul Eggert : > Probably not for -O0. I'm not so sure for -Og. Either way, we shouldn't rely > on > GCC's current behavior in this area as it is neither documented nor guaranteed > to stay the same. Agreed. > > #include > > #include "verify.h" > > > > #ifdef NDEBUG > > # define checked_assume(E) assume (E) > > #else > > # define checked_assume(E) assert (E) > > #endif > > Something like that would work, though the name "checked_assume" is misleading > since the assumption is not always checked. > > "affirm (E)" would be a better name, since the name's not being used anymore > by > the old software verification project[1] and it slides in well next to > "assume" > and "assert". (Some day we're going to run out of synonyms. :-) Believe it or not, but when I first proposed the (initial version of the) macro, I wanted to name it "affirm" after I had looked for synonyms. Only eventually, I switched to the name "checked_assume". But if "affirm" is fine with you, I would love to see it in a module. Either in verify or assure or in a new module named affirm.
Re: stack module
On 5/23/20 9:54 AM, Marc Nieper-Wißkirchen wrote: > Sure, but not in "-O0" or "-Og" builds, I think. Or am I mistaken? Probably not for -O0. I'm not so sure for -Og. Either way, we shouldn't rely on GCC's current behavior in this area as it is neither documented nor guaranteed to stay the same. > #include > #include "verify.h" > > #ifdef NDEBUG > # define checked_assume(E) assume (E) > #else > # define checked_assume(E) assert (E) > #endif Something like that would work, though the name "checked_assume" is misleading since the assumption is not always checked. "affirm (E)" would be a better name, since the name's not being used anymore by the old software verification project[1] and it slides in well next to "assume" and "assert". (Some day we're going to run out of synonyms. :-) [1] http://www.softwarepreservation.org/projects/verification/affirm/
Re: stack module
Marc Nieper-Wißkirchen wrote:
> > I was expecting that you write
> >
> > struct
> > {
> > void *base; ...
> > }
>
> This removes type safety. The benefit of the current approach is that
> stack types of different types are not compatible.
Indeed. Yes, it's a difficult trade-off between debuggability, binary code size,
and type safety...
Bruno
Re: stack module
Am Sa., 23. Mai 2020 um 18:44 Uhr schrieb Paul Eggert :
>
> On 5/23/20 8:55 AM, Marc Nieper-Wißkirchen wrote:
> > A combination of assure and assume would be helpful:
> >
> > #define checked_assume(X) do { assure (X); assume (X); } while (0)
>
> No, because the compiler is entitled to optimize away the 'assure (X)' in this
> case. I installed the attached to try to explain this better.
Sure, but not in "-O0" or "-Og" builds, I think. Or am I mistaken?
But it is definitely better to make it more robust and not rely on
optimization levels:
#include
#include "verify.h"
#ifdef NDEBUG
# define checked_assume(E) assume (E)
#else
# define checked_assume(E) assert (E)
#endif
Re: stack module
On 5/23/20 8:55 AM, Marc Nieper-Wißkirchen wrote:
> A combination of assure and assume would be helpful:
>
> #define checked_assume(X) do { assure (X); assume (X); } while (0)
No, because the compiler is entitled to optimize away the 'assure (X)' in this
case. I installed the attached to try to explain this better.
>From b2c8d02c9750f335549781d20fa37415c9a1edb3 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Sat, 23 May 2020 09:41:54 -0700
Subject: [PATCH] =?UTF-8?q?verify:=20document=20=E2=80=98assume=E2=80=99?=
=?UTF-8?q?=20better?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/verify.h (assume): Say it’s for static analysis, not dynamic.
---
ChangeLog| 5 +
lib/verify.h | 20
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index a4473284c..44450a354 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2020-05-23 Paul Eggert
+
+ verify: document ‘assume’ better
+ * lib/verify.h (assume): Say it’s for static analysis, not dynamic.
+
2020-05-22 Asher Gordon
gendocs: Clarify licenses for templates.
diff --git a/lib/verify.h b/lib/verify.h
index d9ab89a57..f10976127 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -277,10 +277,22 @@ template
#endif
/* Assume that R always holds. Behavior is undefined if R is false,
- fails to evaluate, or has side effects. Although assuming R can
- help a compiler generate better code or diagnostics, performance
- can suffer if R uses hard-to-optimize features such as function
- calls not inlined by the compiler. */
+ fails to evaluate, or has side effects.
+
+ 'assume (R)' is a directive from the programmer telling the
+ compiler that R is true so the compiler needn't generate code to
+ test R. This is why 'assume' is in verify.h: it's related to
+ static checking (in this case, static checking done by the
+ programmer), not dynamic checking.
+
+ 'assume (R)' can affect compilation of all the code, not just code
+ that happens to be executed after the assume (R) is "executed".
+ For example, if the code mistakenly does 'assert (R); assume (R);'
+ the compiler is entitled to optimize away the 'assert (R)'.
+
+ Although assuming R can help a compiler generate better code or
+ diagnostics, performance can suffer if R uses hard-to-optimize
+ features such as function calls not inlined by the compiler. */
#if _GL_HAS_BUILTIN_UNREACHABLE
# define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
--
2.17.1
Re: stack module
Am Sa., 23. Mai 2020 um 17:39 Uhr schrieb Paul Eggert :
>
> On 5/23/20 7:36 AM, Marc Nieper-Wißkirchen wrote:
> > The verify also contains a runtime check `assume', which uses
> > __builtin_unreachable if available to help the compiler in optimizing
> > modes.
>
> I wouldn't call 'assume (X)' a "runtime check". It's more an
> anti-runtime-check.
>
> 'assume (X)' is a directive from the programmer to the compiler that X is true
> so that the compiler needn't generate code to test whether X is true. This is
> why 'assume' is in verify.h: it's related to static checking (in this case,
> static checking done by the programmer), not dynamic checking.
>
> Perhaps I should add a comment to this effect
A combination of assure and assume would be helpful:
#define checked_assume(X) do { assure (X); assume (X); } while (0)
In debug builds, we get assertions and can check our assumptions. In
non-debug (NDEBUG) builds, we just have the compiler hints.
Re: stack module
On 5/23/20 7:36 AM, Marc Nieper-Wißkirchen wrote: > The verify also contains a runtime check `assume', which uses > __builtin_unreachable if available to help the compiler in optimizing > modes. I wouldn't call 'assume (X)' a "runtime check". It's more an anti-runtime-check. 'assume (X)' is a directive from the programmer to the compiler that X is true so that the compiler needn't generate code to test whether X is true. This is why 'assume' is in verify.h: it's related to static checking (in this case, static checking done by the programmer), not dynamic checking. Perhaps I should add a comment to this effect
Re: stack module
Am Sa., 23. Mai 2020 um 16:02 Uhr schrieb Bruno Haible :
> I was expecting that you write
>
> struct
> {
> void *base; ...
> }
This removes type safety. The benefit of the current approach is that
stack types of different types are not compatible.
> Then macro should better be called STACK_FREE or STACK_DESTROY.
> The name STACK_CLEAR suggests that the result is still a valid stack, and
> empty.
Thank you for this suggestion; much better.
> > In the form of assure of the assure module? Or, to facilitate
> > optimization, better assume from verify module? In non-debug builds, I
> > want to make sure that no superfluous checks are made.
>
> The 'verify' module is for compile-time checks.
>
> 'assure' is an improved form of 'assert'.
The verify also contains a runtime check `assume', which uses
__builtin_unreachable if available to help the compiler in optimizing
modes.
Re: stack module
Hi Marc,
> > In Gnulib, we usually avoid extensive use of multi-line macros, because
> > it hampers debuggability. Here, however, one needs macros, in order to
> > accommodate the TYPE argument, which is not necessarily compatible to 'void
> > *'.
> > Nevertheless, we would try to put as much code as possible into functions.
> > The STACK_INIT macro, for example, could be implemented as a function.
>
> How? This would mean that the function stack_init has to take a void *
> argument, which has to be cast to
>
> struct
> {
> void *base; ...
> }
>
> and we have to hope that this structure is guaranteed to be compatible to
>
> struct
> {
> type *base; ...
> }
>
> by the C standard.
I was expecting that you write
struct
{
void *base; ...
}
and the cast the base to 'type *' each time you fetch it. I don't think this
would go the C standard, nor defeat any possible compiler optimization.
> > > #define STACK_CLEAR(stack)\
> > > free ((stack).base)
> >
> > Shouldn't this one also set .size and .allocated to 0 ?
>
> A stack can be uninitialized or initialized. An uninitialized stack is
> initialized by STACK_INIT. It is cleared (and uninitialized) by
> STACK_CLEAR. An uninitialized stack does not have to maintain any
> invariant. The only way to use it is to initialize it again. Thus,
> setting .size and .allocated seems pointless.
Then macro should better be called STACK_FREE or STACK_DESTROY.
The name STACK_CLEAR suggests that the result is still a valid stack, and empty.
> > > #define STACK_POP(stack)\
> > > (stack).base [--(stack).size]
> > >
> > > #define STACK_DISCARD(stack)\
> > > (--(stack).size)
> > >
> > > #define STACK_TOP(stack)\
> > > (stack).base[(stack).size - 1]
> >
> > In these three macros, I would consider to abort() when (stack).size == 0.
>
> In the form of assure of the assure module? Or, to facilitate
> optimization, better assume from verify module? In non-debug builds, I
> want to make sure that no superfluous checks are made.
The 'verify' module is for compile-time checks.
'assure' is an improved form of 'assert'.
Bruno
Re: stack module
Hi Bruno,
> In Gnulib, we usually avoid extensive use of multi-line macros, because
> it hampers debuggability. Here, however, one needs macros, in order to
> accommodate the TYPE argument, which is not necessarily compatible to 'void
> *'.
> Nevertheless, we would try to put as much code as possible into functions.
> The STACK_INIT macro, for example, could be implemented as a function.
How? This would mean that the function stack_init has to take a void *
argument, which has to be cast to
struct
{
void *base; ...
}
and we have to hope that this structure is guaranteed to be compatible to
struct
{
type *base; ...
}
by the C standard.
> > #define STACK_CLEAR(stack)\
> > free ((stack).base)
>
> Shouldn't this one also set .size and .allocated to 0 ?
A stack can be uninitialized or initialized. An uninitialized stack is
initialized by STACK_INIT. It is cleared (and uninitialized) by
STACK_CLEAR. An uninitialized stack does not have to maintain any
invariant. The only way to use it is to initialize it again. Thus,
setting .size and .allocated seems pointless.
> > #define STACK_POP(stack)\
> > (stack).base [--(stack).size]
> >
> > #define STACK_DISCARD(stack)\
> > (--(stack).size)
> >
> > #define STACK_TOP(stack)\
> > (stack).base[(stack).size - 1]
>
> In these three macros, I would consider to abort() when (stack).size == 0.
In the form of assure of the assure module? Or, to facilitate
optimization, better assume from verify module? In non-debug builds, I
want to make sure that no superfluous checks are made.
Marc
Re: stack module
Hi Marc, > Alternatively, one could implement a universally usable stack through > the following header file (mimicking somewhat C++ templates). What do > you think? It will be a lot faster than using the general list modules > of Gnulib. I agree that a generic 'stack' module is useful. I also agree that a single implementation, based on an array, is the way to go. Then it is already faster than the generic list module. In Gnulib, we usually avoid extensive use of multi-line macros, because it hampers debuggability. Here, however, one needs macros, in order to accommodate the TYPE argument, which is not necessarily compatible to 'void *'. Nevertheless, we would try to put as much code as possible into functions. The STACK_INIT macro, for example, could be implemented as a function. > #define STACK_CLEAR(stack)\ > free ((stack).base) Shouldn't this one also set .size and .allocated to 0 ? > #define STACK_POP(stack)\ > (stack).base [--(stack).size] > > #define STACK_DISCARD(stack)\ > (--(stack).size) > > #define STACK_TOP(stack)\ > (stack).base[(stack).size - 1] In these three macros, I would consider to abort() when (stack).size == 0. Bruno
