* [PATCH 1/2] darray: Fix bug in the darray_remove() macro @ 2017-08-26 16:26 Damien Grassart 2017-08-27 2:56 ` David Gibson 0 siblings, 1 reply; 11+ messages in thread From: Damien Grassart @ 2017-08-26 16:26 UTC (permalink / raw) To: ccan The memmove() call should be using the index argument to determine the number of bytes to copy. Signed-off-by: Damien Grassart <damien@grassart.com> --- ccan/darray/darray.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h index 75112419..8d47645b 100644 --- a/ccan/darray/darray.h +++ b/ccan/darray/darray.h @@ -225,7 +225,7 @@ typedef darray(unsigned long) darray_ulong; /* Warning, slow: Requires copying all elements after removed item. */ #define darray_remove(arr, index) do { \ if (index < arr.size-1) \ - memmove(&(arr).item[index], &(arr).item[index+1], ((arr).size-1-i)*sizeof(*(arr).item)); \ + memmove(&(arr).item[index], &(arr).item[index+1], ((arr).size-1-index)*sizeof(*(arr).item)); \ (arr).size--; \ } while(0) -- 2.14.1 _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] darray: Fix bug in the darray_remove() macro 2017-08-26 16:26 [PATCH 1/2] darray: Fix bug in the darray_remove() macro Damien Grassart @ 2017-08-27 2:56 ` David Gibson 2017-08-27 21:02 ` Damien Grassart 2017-08-27 21:26 ` [PATCH 1/2] darray: Rename identifiers starting with an underscore Damien Grassart 0 siblings, 2 replies; 11+ messages in thread From: David Gibson @ 2017-08-27 2:56 UTC (permalink / raw) To: Damien Grassart; +Cc: ccan [-- Attachment #1.1: Type: text/plain, Size: 1438 bytes --] On Sat, Aug 26, 2017 at 06:26:20PM +0200, Damien Grassart wrote: > The memmove() call should be using the index argument to determine the > number of bytes to copy. > > Signed-off-by: Damien Grassart <damien@grassart.com> No question that was a bug. Fix applied. Note for a possible future cleanup: calling identifiers 'index' is usually a bad idea, because it shadows index(3) in the C library, which means you can get really confusing errors (or lack of errors) if you remove the declaration but not the users. > --- > ccan/darray/darray.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h > index 75112419..8d47645b 100644 > --- a/ccan/darray/darray.h > +++ b/ccan/darray/darray.h > @@ -225,7 +225,7 @@ typedef darray(unsigned long) darray_ulong; > /* Warning, slow: Requires copying all elements after removed item. */ > #define darray_remove(arr, index) do { \ > if (index < arr.size-1) \ > - memmove(&(arr).item[index], &(arr).item[index+1], ((arr).size-1-i)*sizeof(*(arr).item)); \ > + memmove(&(arr).item[index], &(arr).item[index+1], ((arr).size-1-index)*sizeof(*(arr).item)); \ > (arr).size--; \ > } while(0) > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] darray: Fix bug in the darray_remove() macro 2017-08-27 2:56 ` David Gibson @ 2017-08-27 21:02 ` Damien Grassart 2017-08-27 21:26 ` [PATCH 1/2] darray: Rename identifiers starting with an underscore Damien Grassart 1 sibling, 0 replies; 11+ messages in thread From: Damien Grassart @ 2017-08-27 21:02 UTC (permalink / raw) To: David Gibson; +Cc: ccan [-- Attachment #1.1: Type: text/plain, Size: 1727 bytes --] Good point about shadowing index(3), I'll send a patch renaming all the uses of 'index'. On Sun, Aug 27, 2017 at 4:56 AM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Sat, Aug 26, 2017 at 06:26:20PM +0200, Damien Grassart wrote: > > The memmove() call should be using the index argument to determine the > > number of bytes to copy. > > > > Signed-off-by: Damien Grassart <damien@grassart.com> > > No question that was a bug. Fix applied. > > Note for a possible future cleanup: calling identifiers 'index' is > usually a bad idea, because it shadows index(3) in the C library, > which means you can get really confusing errors (or lack of errors) if > you remove the declaration but not the users. > > > --- > > ccan/darray/darray.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h > > index 75112419..8d47645b 100644 > > --- a/ccan/darray/darray.h > > +++ b/ccan/darray/darray.h > > @@ -225,7 +225,7 @@ typedef darray(unsigned long) darray_ulong; > > /* Warning, slow: Requires copying all elements after removed item. */ > > #define darray_remove(arr, index) do { \ > > if (index < arr.size-1) \ > > - memmove(&(arr).item[index], &(arr).item[index+1], > ((arr).size-1-i)*sizeof(*(arr).item)); \ > > + memmove(&(arr).item[index], &(arr).item[index+1], > ((arr).size-1-index)*sizeof(*(arr).item)); \ > > (arr).size--; \ > > } while(0) > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ > _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson > [-- Attachment #1.2: Type: text/html, Size: 2640 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] darray: Rename identifiers starting with an underscore 2017-08-27 2:56 ` David Gibson 2017-08-27 21:02 ` Damien Grassart @ 2017-08-27 21:26 ` Damien Grassart 2017-08-27 21:26 ` [PATCH 2/2] darray: Fix bug in the darray_remove() macro Damien Grassart 2017-08-28 2:43 ` [PATCH 1/2] darray: Rename identifiers starting with an underscore David Gibson 1 sibling, 2 replies; 11+ messages in thread From: Damien Grassart @ 2017-08-27 21:26 UTC (permalink / raw) To: ccan Identifiers starting with underscores are technically reserved for system use, so rename all of them to end with one instead. Signed-off-by: Damien Grassart <damien@grassart.com> --- ccan/darray/darray.h | 68 ++++++++++++++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h index fca20b8a..82726c05 100644 --- a/ccan/darray/darray.h +++ b/ccan/darray/darray.h @@ -171,10 +171,10 @@ typedef darray(unsigned long) darray_ulong; (arr).item[0] = (__VA_ARGS__); \ } while(0) #define darray_insert(arr, index, ...) do { \ - size_t __index = index; \ + size_t index_ = index; \ darray_resize(arr, (arr).size+1); \ - memmove((arr).item+__index+1, (arr).item+__index, ((arr).size-__index-1)*sizeof(*(arr).item)); \ - (arr).item[__index] = (__VA_ARGS__); \ + memmove((arr).item+index_+1, (arr).item+index_, ((arr).size-index_-1)*sizeof(*(arr).item)); \ + (arr).item[index_] = (__VA_ARGS__); \ } while(0) #define darray_push(arr, ...) darray_append(arr, __VA_ARGS__) @@ -182,30 +182,30 @@ typedef darray(unsigned long) darray_ulong; /*** Insertion (multiple items) ***/ #define darray_append_items(arr, items, count) do { \ - size_t __count = (count), __oldSize = (arr).size; \ - darray_resize(arr, __oldSize + __count); \ - memcpy((arr).item + __oldSize, items, __count * sizeof(*(arr).item)); \ + size_t count_ = (count), oldSize_ = (arr).size; \ + darray_resize(arr, oldSize_ + count_); \ + memcpy((arr).item + oldSize_, items, count_ * sizeof(*(arr).item)); \ } while(0) #define darray_prepend_items(arr, items, count) do { \ - size_t __count = (count), __oldSize = (arr).size; \ - darray_resize(arr, __count + __oldSize); \ - memmove((arr).item + __count, (arr).item, __oldSize * sizeof(*(arr).item)); \ - memcpy((arr).item, items, __count * sizeof(*(arr).item)); \ + size_t count_ = (count), oldSize_ = (arr).size; \ + darray_resize(arr, count_ + oldSize_); \ + memmove((arr).item + count_, (arr).item, oldSize_ * sizeof(*(arr).item)); \ + memcpy((arr).item, items, count_ * sizeof(*(arr).item)); \ } while(0) #define darray_append_items_nullterminate(arr, items, count) do { \ - size_t __count = (count), __oldSize = (arr).size; \ - darray_resize(arr, __oldSize + __count + 1); \ - memcpy((arr).item + __oldSize, items, __count * sizeof(*(arr).item)); \ + size_t count_ = (count), oldSize_ = (arr).size; \ + darray_resize(arr, oldSize_ + count_ + 1); \ + memcpy((arr).item + oldSize_, items, count_ * sizeof(*(arr).item)); \ (arr).item[--(arr).size] = 0; \ } while(0) #define darray_prepend_items_nullterminate(arr, items, count) do { \ - size_t __count = (count), __oldSize = (arr).size; \ - darray_resize(arr, __count + __oldSize + 1); \ - memmove((arr).item + __count, (arr).item, __oldSize * sizeof(*(arr).item)); \ - memcpy((arr).item, items, __count * sizeof(*(arr).item)); \ + size_t count_ = (count), oldSize_ = (arr).size; \ + darray_resize(arr, count_ + oldSize_ + 1); \ + memmove((arr).item + count_, (arr).item, oldSize_ * sizeof(*(arr).item)); \ + memcpy((arr).item, items, count_ * sizeof(*(arr).item)); \ (arr).item[--(arr).size] = 0; \ } while(0) @@ -215,12 +215,12 @@ typedef darray(unsigned long) darray_ulong; #endif #define darray_appends_t(arr, type, ...) do { \ - type __src[] = {__VA_ARGS__}; \ - darray_append_items(arr, __src, sizeof(__src)/sizeof(*__src)); \ + type src_[] = {__VA_ARGS__}; \ + darray_append_items(arr, src_, sizeof(src_)/sizeof(*src_)); \ } while(0) #define darray_prepends_t(arr, type, ...) do { \ - type __src[] = {__VA_ARGS__}; \ - darray_prepend_items(arr, __src, sizeof(__src)/sizeof(*__src)); \ + type src_[] = {__VA_ARGS__}; \ + darray_prepend_items(arr, src_, sizeof(src_)/sizeof(*src_)); \ } while(0) @@ -239,23 +239,23 @@ typedef darray(unsigned long) darray_ulong; /*** Replacement ***/ -#define darray_from_items(arr, items, count) do {size_t __count = (count); darray_resize(arr, __count); memcpy((arr).item, items, __count*sizeof(*(arr).item));} while(0) +#define darray_from_items(arr, items, count) do {size_t count_ = (count); darray_resize(arr, count_); memcpy((arr).item, items, count_*sizeof(*(arr).item));} while(0) #define darray_from_c(arr, c_array) darray_from_items(arr, c_array, sizeof(c_array)/sizeof(*(c_array))) /*** String buffer ***/ -#define darray_append_string(arr, str) do {const char *__str = (str); darray_append_items(arr, __str, strlen(__str)+1); (arr).size--;} while(0) +#define darray_append_string(arr, str) do {const char *str_ = (str); darray_append_items(arr, str_, strlen(str_)+1); (arr).size--;} while(0) #define darray_append_lit(arr, stringLiteral) do {darray_append_items(arr, stringLiteral, sizeof(stringLiteral)); (arr).size--;} while(0) #define darray_prepend_string(arr, str) do { \ - const char *__str = (str); \ - darray_prepend_items_nullterminate(arr, __str, strlen(__str)); \ + const char *str_ = (str); \ + darray_prepend_items_nullterminate(arr, str_, strlen(str_)); \ } while(0) #define darray_prepend_lit(arr, stringLiteral) \ darray_prepend_items_nullterminate(arr, stringLiteral, sizeof(stringLiteral) - 1) -#define darray_from_string(arr, str) do {const char *__str = (str); darray_from_items(arr, __str, strlen(__str)+1); (arr).size--;} while(0) +#define darray_from_string(arr, str) do {const char *str_ = (str); darray_from_items(arr, str_, strlen(str_)+1); (arr).size--;} while(0) #define darray_from_lit(arr, stringLiteral) do {darray_from_items(arr, stringLiteral, sizeof(stringLiteral)); (arr).size--;} while(0) @@ -263,11 +263,11 @@ typedef darray(unsigned long) darray_ulong; #define darray_resize(arr, newSize) darray_growalloc(arr, (arr).size = (newSize)) #define darray_resize0(arr, newSize) do { \ - size_t __oldSize = (arr).size, __newSize = (newSize); \ - (arr).size = __newSize; \ - if (__newSize > __oldSize) { \ - darray_growalloc(arr, __newSize); \ - memset(&(arr).item[__oldSize], 0, (__newSize - __oldSize) * sizeof(*(arr).item)); \ + size_t oldSize_ = (arr).size, newSize_ = (newSize); \ + (arr).size = newSize_; \ + if (newSize_ > oldSize_) { \ + darray_growalloc(arr, newSize_); \ + memset(&(arr).item[oldSize_], 0, (newSize_ - oldSize_) * sizeof(*(arr).item)); \ } \ } while(0) @@ -275,9 +275,9 @@ typedef darray(unsigned long) darray_ulong; (arr).item = realloc((arr).item, ((arr).alloc = (newAlloc)) * sizeof(*(arr).item)); \ } while(0) #define darray_growalloc(arr, need) do { \ - size_t __need = (need); \ - if (__need > (arr).alloc) \ - darray_realloc(arr, darray_next_alloc((arr).alloc, __need)); \ + size_t need_ = (need); \ + if (need_ > (arr).alloc) \ + darray_realloc(arr, darray_next_alloc((arr).alloc, need_)); \ } while(0) #if HAVE_STATEMENT_EXPR==1 -- 2.14.1 _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] darray: Fix bug in the darray_remove() macro 2017-08-27 21:26 ` [PATCH 1/2] darray: Rename identifiers starting with an underscore Damien Grassart @ 2017-08-27 21:26 ` Damien Grassart 2017-08-28 2:48 ` David Gibson 2017-08-28 2:43 ` [PATCH 1/2] darray: Rename identifiers starting with an underscore David Gibson 1 sibling, 1 reply; 11+ messages in thread From: Damien Grassart @ 2017-08-27 21:26 UTC (permalink / raw) To: ccan The memmove() call should be using the index argument to determine the number of bytes to copy. To be consistent with the rest of the code, we should also not evaluate the index parameter multiple times. Calling this with rand() % arr.size would otherwise generally segfault. Finally, we want to avoid using "index" as an identifier so as to not shadow index(3) in the C library. Signed-off-by: Damien Grassart <damien@grassart.com> --- ccan/darray/darray.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h index 82726c05..6787f14c 100644 --- a/ccan/darray/darray.h +++ b/ccan/darray/darray.h @@ -170,8 +170,8 @@ typedef darray(unsigned long) darray_ulong; memmove((arr).item+1, (arr).item, ((arr).size-1)*sizeof(*(arr).item)); \ (arr).item[0] = (__VA_ARGS__); \ } while(0) -#define darray_insert(arr, index, ...) do { \ - size_t index_ = index; \ +#define darray_insert(arr, i, ...) do { \ + size_t index_ = i; \ darray_resize(arr, (arr).size+1); \ memmove((arr).item+index_+1, (arr).item+index_, ((arr).size-index_-1)*sizeof(*(arr).item)); \ (arr).item[index_] = (__VA_ARGS__); \ @@ -230,9 +230,10 @@ typedef darray(unsigned long) darray_ulong; #define darray_pop(arr) ((arr).item[--(arr).size]) #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NULL) /* Warning, slow: Requires copying all elements after removed item. */ -#define darray_remove(arr, index) do { \ - if (index < arr.size-1) \ - memmove(&(arr).item[index], &(arr).item[index+1], ((arr).size-1-i)*sizeof(*(arr).item)); \ +#define darray_remove(arr, i) do { \ + size_t index_ = i; + if (index_ < arr.size-1) \ + memmove(&(arr).item[index_], &(arr).item[index_+1], ((arr).size-1-index_)*sizeof(*(arr).item)); \ (arr).size--; \ } while(0) -- 2.14.1 _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] darray: Fix bug in the darray_remove() macro 2017-08-27 21:26 ` [PATCH 2/2] darray: Fix bug in the darray_remove() macro Damien Grassart @ 2017-08-28 2:48 ` David Gibson 2017-08-28 5:03 ` Damien Grassart 2017-08-28 5:09 ` [PATCH] " Damien Grassart 0 siblings, 2 replies; 11+ messages in thread From: David Gibson @ 2017-08-28 2:48 UTC (permalink / raw) To: Damien Grassart; +Cc: ccan [-- Attachment #1.1: Type: text/plain, Size: 3501 bytes --] On Sun, Aug 27, 2017 at 11:26:24PM +0200, Damien Grassart wrote: > The memmove() call should be using the index argument to determine the > number of bytes to copy. To be consistent with the rest of the code, > we should also not evaluate the index parameter multiple > times. Calling this with rand() % arr.size would otherwise generally > segfault. > > Finally, we want to avoid using "index" as an identifier so as to not > shadow index(3) in the C library. > > Signed-off-by: Damien Grassart <damien@grassart.com> This breaks compile for me, though I can't quickly see why. $ make cc -g3 -ggdb -Wall -Wstrict-prototypes -Wold-style-definition -Wundef -Wmissing-prototypes -Wmissing-declarations -Wpointer-arith -Wwrite-strings -DCCAN_STR_DEBUG=1 -I. -MMD -MP -MFccan/strgrp/strgrp.o.d -MTccan/strgrp/strgrp.o -c ccan/strgrp/strgrp.c -o ccan/strgrp/strgrp.o In file included from ccan/strgrp/strgrp.c:26:0: ./ccan/darray/darray.h:235:2: error: expected identifier or ‘(’ before ‘if’ if (index_ < arr.size-1) \ ^~ ./ccan/darray/darray.h:237:7: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘.’ token (arr).size--; \ ^ ./ccan/darray/darray.h:238:2: error: expected identifier or ‘(’ before ‘}’ token } while(0) ^ ./ccan/darray/darray.h:238:4: error: expected identifier or ‘(’ before ‘while’ } while(0) ^~~~~ I also noticed that both this and the earlier patches use: size_t index_ = i; in the macro, without parentheses around the 'i' macro paramater. That's not the cause of the error above, but it's not good practice as a rule. So, I've backed out these darray patches for now. Can you debug the compile problem above and resend the whole lot as a single series. > --- > ccan/darray/darray.h | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h > index 82726c05..6787f14c 100644 > --- a/ccan/darray/darray.h > +++ b/ccan/darray/darray.h > @@ -170,8 +170,8 @@ typedef darray(unsigned long) darray_ulong; > memmove((arr).item+1, (arr).item, ((arr).size-1)*sizeof(*(arr).item)); \ > (arr).item[0] = (__VA_ARGS__); \ > } while(0) > -#define darray_insert(arr, index, ...) do { \ > - size_t index_ = index; \ > +#define darray_insert(arr, i, ...) do { \ > + size_t index_ = i; \ > darray_resize(arr, (arr).size+1); \ > memmove((arr).item+index_+1, (arr).item+index_, ((arr).size-index_-1)*sizeof(*(arr).item)); \ > (arr).item[index_] = (__VA_ARGS__); \ > @@ -230,9 +230,10 @@ typedef darray(unsigned long) darray_ulong; > #define darray_pop(arr) ((arr).item[--(arr).size]) > #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NULL) > /* Warning, slow: Requires copying all elements after removed item. */ > -#define darray_remove(arr, index) do { \ > - if (index < arr.size-1) \ > - memmove(&(arr).item[index], &(arr).item[index+1], ((arr).size-1-i)*sizeof(*(arr).item)); \ > +#define darray_remove(arr, i) do { \ > + size_t index_ = i; > + if (index_ < arr.size-1) \ > + memmove(&(arr).item[index_], &(arr).item[index_+1], ((arr).size-1-index_)*sizeof(*(arr).item)); \ > (arr).size--; \ > } while(0) > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] darray: Fix bug in the darray_remove() macro 2017-08-28 2:48 ` David Gibson @ 2017-08-28 5:03 ` Damien Grassart 2017-08-28 5:09 ` [PATCH] " Damien Grassart 1 sibling, 0 replies; 11+ messages in thread From: Damien Grassart @ 2017-08-28 5:03 UTC (permalink / raw) To: David Gibson; +Cc: ccan [-- Attachment #1.1: Type: text/plain, Size: 4204 bytes --] Ugh, sorry about that, I forgot to run the tests after the last patch fixing darray_remove(). I missed a backslash in that patch (darray: Fix bug in the darray_remove() macro). I'll resend it with the missing parentheses around the macro params. Thanks. On Mon, Aug 28, 2017 at 4:48 AM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Sun, Aug 27, 2017 at 11:26:24PM +0200, Damien Grassart wrote: > > The memmove() call should be using the index argument to determine the > > number of bytes to copy. To be consistent with the rest of the code, > > we should also not evaluate the index parameter multiple > > times. Calling this with rand() % arr.size would otherwise generally > > segfault. > > > > Finally, we want to avoid using "index" as an identifier so as to not > > shadow index(3) in the C library. > > > > Signed-off-by: Damien Grassart <damien@grassart.com> > > This breaks compile for me, though I can't quickly see why. > > $ make > cc -g3 -ggdb -Wall -Wstrict-prototypes -Wold-style-definition -Wundef > -Wmissing-prototypes -Wmissing-declarations -Wpointer-arith -Wwrite-strings > -DCCAN_STR_DEBUG=1 -I. -MMD -MP -MFccan/strgrp/strgrp.o.d > -MTccan/strgrp/strgrp.o -c ccan/strgrp/strgrp.c -o ccan/strgrp/strgrp.o > In file included from ccan/strgrp/strgrp.c:26:0: > ./ccan/darray/darray.h:235:2: error: expected identifier or ‘(’ before ‘if’ > if (index_ < arr.size-1) \ > ^~ > ./ccan/darray/darray.h:237:7: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or > ‘__attribute__’ before ‘.’ token > (arr).size--; \ > ^ > ./ccan/darray/darray.h:238:2: error: expected identifier or ‘(’ before ‘}’ > token > } while(0) > ^ > ./ccan/darray/darray.h:238:4: error: expected identifier or ‘(’ before > ‘while’ > } while(0) > ^~~~~ > > I also noticed that both this and the earlier patches use: > size_t index_ = i; > in the macro, without parentheses around the 'i' macro paramater. > That's not the cause of the error above, but it's not good practice as > a rule. > > So, I've backed out these darray patches for now. Can you debug the > compile problem above and resend the whole lot as a single series. > > > --- > > ccan/darray/darray.h | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h > > index 82726c05..6787f14c 100644 > > --- a/ccan/darray/darray.h > > +++ b/ccan/darray/darray.h > > @@ -170,8 +170,8 @@ typedef darray(unsigned long) darray_ulong; > > memmove((arr).item+1, (arr).item, > ((arr).size-1)*sizeof(*(arr).item)); \ > > (arr).item[0] = (__VA_ARGS__); \ > > } while(0) > > -#define darray_insert(arr, index, ...) do { \ > > - size_t index_ = index; \ > > +#define darray_insert(arr, i, ...) do { \ > > + size_t index_ = i; \ > > darray_resize(arr, (arr).size+1); \ > > memmove((arr).item+index_+1, (arr).item+index_, > ((arr).size-index_-1)*sizeof(*(arr).item)); \ > > (arr).item[index_] = (__VA_ARGS__); \ > > @@ -230,9 +230,10 @@ typedef darray(unsigned long) darray_ulong; > > #define darray_pop(arr) ((arr).item[--(arr).size]) > > #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NULL) > > /* Warning, slow: Requires copying all elements after removed item. */ > > -#define darray_remove(arr, index) do { \ > > - if (index < arr.size-1) \ > > - memmove(&(arr).item[index], &(arr).item[index+1], > ((arr).size-1-i)*sizeof(*(arr).item)); \ > > +#define darray_remove(arr, i) do { \ > > + size_t index_ = i; > > + if (index_ < arr.size-1) \ > > + memmove(&(arr).item[index_], &(arr).item[index_+1], > ((arr).size-1-index_)*sizeof(*(arr).item)); \ > > (arr).size--; \ > > } while(0) > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ > _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson > [-- Attachment #1.2: Type: text/html, Size: 5309 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] darray: Fix bug in the darray_remove() macro 2017-08-28 2:48 ` David Gibson 2017-08-28 5:03 ` Damien Grassart @ 2017-08-28 5:09 ` Damien Grassart 2017-08-29 4:54 ` David Gibson 1 sibling, 1 reply; 11+ messages in thread From: Damien Grassart @ 2017-08-28 5:09 UTC (permalink / raw) To: ccan The memmove() call should be using the index argument to determine the number of bytes to copy. To be consistent with the rest of the code, we should also not evaluate the index parameter multiple times. Calling this with rand() % arr.size would otherwise generally segfault. Finally, we want to avoid using "index" as an identifier so as to not shadow index(3) in the C library. Signed-off-by: Damien Grassart <damien@grassart.com> --- ccan/darray/darray.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h index 82726c05..58470fde 100644 --- a/ccan/darray/darray.h +++ b/ccan/darray/darray.h @@ -170,8 +170,8 @@ typedef darray(unsigned long) darray_ulong; memmove((arr).item+1, (arr).item, ((arr).size-1)*sizeof(*(arr).item)); \ (arr).item[0] = (__VA_ARGS__); \ } while(0) -#define darray_insert(arr, index, ...) do { \ - size_t index_ = index; \ +#define darray_insert(arr, i, ...) do { \ + size_t index_ = (i); \ darray_resize(arr, (arr).size+1); \ memmove((arr).item+index_+1, (arr).item+index_, ((arr).size-index_-1)*sizeof(*(arr).item)); \ (arr).item[index_] = (__VA_ARGS__); \ @@ -230,9 +230,10 @@ typedef darray(unsigned long) darray_ulong; #define darray_pop(arr) ((arr).item[--(arr).size]) #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NULL) /* Warning, slow: Requires copying all elements after removed item. */ -#define darray_remove(arr, index) do { \ - if (index < arr.size-1) \ - memmove(&(arr).item[index], &(arr).item[index+1], ((arr).size-1-i)*sizeof(*(arr).item)); \ +#define darray_remove(arr, i) do { \ + size_t index_ = (i); \ + if (index_ < arr.size-1) \ + memmove(&(arr).item[index_], &(arr).item[index_+1], ((arr).size-1-index_)*sizeof(*(arr).item)); \ (arr).size--; \ } while(0) -- 2.14.1 _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] darray: Fix bug in the darray_remove() macro 2017-08-28 5:09 ` [PATCH] " Damien Grassart @ 2017-08-29 4:54 ` David Gibson 2017-08-29 10:08 ` Damien Grassart 0 siblings, 1 reply; 11+ messages in thread From: David Gibson @ 2017-08-29 4:54 UTC (permalink / raw) To: Damien Grassart; +Cc: ccan [-- Attachment #1.1: Type: text/plain, Size: 2479 bytes --] On Mon, Aug 28, 2017 at 07:09:35AM +0200, Damien Grassart wrote: > The memmove() call should be using the index argument to determine the > number of bytes to copy. To be consistent with the rest of the code, > we should also not evaluate the index parameter multiple > times. Calling this with rand() % arr.size would otherwise generally > segfault. > > Finally, we want to avoid using "index" as an identifier so as to not > shadow index(3) in the C library. Uh.. sorry, I think we're in a state of confusion because applied some of the patches then removed them again due to problems discovered later. Can you please rebase on the latest ccan tree and send me the whole set of patches as a batch. > > Signed-off-by: Damien Grassart <damien@grassart.com> > --- > ccan/darray/darray.h | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h > index 82726c05..58470fde 100644 > --- a/ccan/darray/darray.h > +++ b/ccan/darray/darray.h > @@ -170,8 +170,8 @@ typedef darray(unsigned long) darray_ulong; > memmove((arr).item+1, (arr).item, ((arr).size-1)*sizeof(*(arr).item)); \ > (arr).item[0] = (__VA_ARGS__); \ > } while(0) > -#define darray_insert(arr, index, ...) do { \ > - size_t index_ = index; \ > +#define darray_insert(arr, i, ...) do { \ > + size_t index_ = (i); \ > darray_resize(arr, (arr).size+1); \ > memmove((arr).item+index_+1, (arr).item+index_, ((arr).size-index_-1)*sizeof(*(arr).item)); \ > (arr).item[index_] = (__VA_ARGS__); \ > @@ -230,9 +230,10 @@ typedef darray(unsigned long) darray_ulong; > #define darray_pop(arr) ((arr).item[--(arr).size]) > #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NULL) > /* Warning, slow: Requires copying all elements after removed item. */ > -#define darray_remove(arr, index) do { \ > - if (index < arr.size-1) \ > - memmove(&(arr).item[index], &(arr).item[index+1], ((arr).size-1-i)*sizeof(*(arr).item)); \ > +#define darray_remove(arr, i) do { \ > + size_t index_ = (i); \ > + if (index_ < arr.size-1) \ > + memmove(&(arr).item[index_], &(arr).item[index_+1], ((arr).size-1-index_)*sizeof(*(arr).item)); \ > (arr).size--; \ > } while(0) > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] darray: Fix bug in the darray_remove() macro 2017-08-29 4:54 ` David Gibson @ 2017-08-29 10:08 ` Damien Grassart 0 siblings, 0 replies; 11+ messages in thread From: Damien Grassart @ 2017-08-29 10:08 UTC (permalink / raw) To: David Gibson; +Cc: ccan [-- Attachment #1.1: Type: text/plain, Size: 2847 bytes --] Sorry about the confusion, I'll resend the whole set. On Tue, Aug 29, 2017 at 6:54 AM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Aug 28, 2017 at 07:09:35AM +0200, Damien Grassart wrote: > > The memmove() call should be using the index argument to determine the > > number of bytes to copy. To be consistent with the rest of the code, > > we should also not evaluate the index parameter multiple > > times. Calling this with rand() % arr.size would otherwise generally > > segfault. > > > > Finally, we want to avoid using "index" as an identifier so as to not > > shadow index(3) in the C library. > > Uh.. sorry, I think we're in a state of confusion because applied some > of the patches then removed them again due to problems discovered > later. > > Can you please rebase on the latest ccan tree and send me the whole > set of patches as a batch. > > > > > Signed-off-by: Damien Grassart <damien@grassart.com> > > --- > > ccan/darray/darray.h | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h > > index 82726c05..58470fde 100644 > > --- a/ccan/darray/darray.h > > +++ b/ccan/darray/darray.h > > @@ -170,8 +170,8 @@ typedef darray(unsigned long) darray_ulong; > > memmove((arr).item+1, (arr).item, > ((arr).size-1)*sizeof(*(arr).item)); \ > > (arr).item[0] = (__VA_ARGS__); \ > > } while(0) > > -#define darray_insert(arr, index, ...) do { \ > > - size_t index_ = index; \ > > +#define darray_insert(arr, i, ...) do { \ > > + size_t index_ = (i); \ > > darray_resize(arr, (arr).size+1); \ > > memmove((arr).item+index_+1, (arr).item+index_, > ((arr).size-index_-1)*sizeof(*(arr).item)); \ > > (arr).item[index_] = (__VA_ARGS__); \ > > @@ -230,9 +230,10 @@ typedef darray(unsigned long) darray_ulong; > > #define darray_pop(arr) ((arr).item[--(arr).size]) > > #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NULL) > > /* Warning, slow: Requires copying all elements after removed item. */ > > -#define darray_remove(arr, index) do { \ > > - if (index < arr.size-1) \ > > - memmove(&(arr).item[index], &(arr).item[index+1], > ((arr).size-1-i)*sizeof(*(arr).item)); \ > > +#define darray_remove(arr, i) do { \ > > + size_t index_ = (i); \ > > + if (index_ < arr.size-1) \ > > + memmove(&(arr).item[index_], &(arr).item[index_+1], > ((arr).size-1-index_)*sizeof(*(arr).item)); \ > > (arr).size--; \ > > } while(0) > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ > _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson > [-- Attachment #1.2: Type: text/html, Size: 3956 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] darray: Rename identifiers starting with an underscore 2017-08-27 21:26 ` [PATCH 1/2] darray: Rename identifiers starting with an underscore Damien Grassart 2017-08-27 21:26 ` [PATCH 2/2] darray: Fix bug in the darray_remove() macro Damien Grassart @ 2017-08-28 2:43 ` David Gibson 1 sibling, 0 replies; 11+ messages in thread From: David Gibson @ 2017-08-28 2:43 UTC (permalink / raw) To: Damien Grassart; +Cc: ccan [-- Attachment #1.1: Type: text/plain, Size: 7598 bytes --] On Sun, Aug 27, 2017 at 11:26:23PM +0200, Damien Grassart wrote: > Identifiers starting with underscores are technically reserved for > system use, so rename all of them to end with one instead. > > Signed-off-by: Damien Grassart <damien@grassart.com> Applied, thanks. > --- > ccan/darray/darray.h | 68 ++++++++++++++++++++++++++-------------------------- > 1 file changed, 34 insertions(+), 34 deletions(-) > > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h > index fca20b8a..82726c05 100644 > --- a/ccan/darray/darray.h > +++ b/ccan/darray/darray.h > @@ -171,10 +171,10 @@ typedef darray(unsigned long) darray_ulong; > (arr).item[0] = (__VA_ARGS__); \ > } while(0) > #define darray_insert(arr, index, ...) do { \ > - size_t __index = index; \ > + size_t index_ = index; \ > darray_resize(arr, (arr).size+1); \ > - memmove((arr).item+__index+1, (arr).item+__index, ((arr).size-__index-1)*sizeof(*(arr).item)); \ > - (arr).item[__index] = (__VA_ARGS__); \ > + memmove((arr).item+index_+1, (arr).item+index_, ((arr).size-index_-1)*sizeof(*(arr).item)); \ > + (arr).item[index_] = (__VA_ARGS__); \ > } while(0) > #define darray_push(arr, ...) darray_append(arr, __VA_ARGS__) > > @@ -182,30 +182,30 @@ typedef darray(unsigned long) darray_ulong; > /*** Insertion (multiple items) ***/ > > #define darray_append_items(arr, items, count) do { \ > - size_t __count = (count), __oldSize = (arr).size; \ > - darray_resize(arr, __oldSize + __count); \ > - memcpy((arr).item + __oldSize, items, __count * sizeof(*(arr).item)); \ > + size_t count_ = (count), oldSize_ = (arr).size; \ > + darray_resize(arr, oldSize_ + count_); \ > + memcpy((arr).item + oldSize_, items, count_ * sizeof(*(arr).item)); \ > } while(0) > > #define darray_prepend_items(arr, items, count) do { \ > - size_t __count = (count), __oldSize = (arr).size; \ > - darray_resize(arr, __count + __oldSize); \ > - memmove((arr).item + __count, (arr).item, __oldSize * sizeof(*(arr).item)); \ > - memcpy((arr).item, items, __count * sizeof(*(arr).item)); \ > + size_t count_ = (count), oldSize_ = (arr).size; \ > + darray_resize(arr, count_ + oldSize_); \ > + memmove((arr).item + count_, (arr).item, oldSize_ * sizeof(*(arr).item)); \ > + memcpy((arr).item, items, count_ * sizeof(*(arr).item)); \ > } while(0) > > #define darray_append_items_nullterminate(arr, items, count) do { \ > - size_t __count = (count), __oldSize = (arr).size; \ > - darray_resize(arr, __oldSize + __count + 1); \ > - memcpy((arr).item + __oldSize, items, __count * sizeof(*(arr).item)); \ > + size_t count_ = (count), oldSize_ = (arr).size; \ > + darray_resize(arr, oldSize_ + count_ + 1); \ > + memcpy((arr).item + oldSize_, items, count_ * sizeof(*(arr).item)); \ > (arr).item[--(arr).size] = 0; \ > } while(0) > > #define darray_prepend_items_nullterminate(arr, items, count) do { \ > - size_t __count = (count), __oldSize = (arr).size; \ > - darray_resize(arr, __count + __oldSize + 1); \ > - memmove((arr).item + __count, (arr).item, __oldSize * sizeof(*(arr).item)); \ > - memcpy((arr).item, items, __count * sizeof(*(arr).item)); \ > + size_t count_ = (count), oldSize_ = (arr).size; \ > + darray_resize(arr, count_ + oldSize_ + 1); \ > + memmove((arr).item + count_, (arr).item, oldSize_ * sizeof(*(arr).item)); \ > + memcpy((arr).item, items, count_ * sizeof(*(arr).item)); \ > (arr).item[--(arr).size] = 0; \ > } while(0) > > @@ -215,12 +215,12 @@ typedef darray(unsigned long) darray_ulong; > #endif > > #define darray_appends_t(arr, type, ...) do { \ > - type __src[] = {__VA_ARGS__}; \ > - darray_append_items(arr, __src, sizeof(__src)/sizeof(*__src)); \ > + type src_[] = {__VA_ARGS__}; \ > + darray_append_items(arr, src_, sizeof(src_)/sizeof(*src_)); \ > } while(0) > #define darray_prepends_t(arr, type, ...) do { \ > - type __src[] = {__VA_ARGS__}; \ > - darray_prepend_items(arr, __src, sizeof(__src)/sizeof(*__src)); \ > + type src_[] = {__VA_ARGS__}; \ > + darray_prepend_items(arr, src_, sizeof(src_)/sizeof(*src_)); \ > } while(0) > > > @@ -239,23 +239,23 @@ typedef darray(unsigned long) darray_ulong; > > /*** Replacement ***/ > > -#define darray_from_items(arr, items, count) do {size_t __count = (count); darray_resize(arr, __count); memcpy((arr).item, items, __count*sizeof(*(arr).item));} while(0) > +#define darray_from_items(arr, items, count) do {size_t count_ = (count); darray_resize(arr, count_); memcpy((arr).item, items, count_*sizeof(*(arr).item));} while(0) > #define darray_from_c(arr, c_array) darray_from_items(arr, c_array, sizeof(c_array)/sizeof(*(c_array))) > > > /*** String buffer ***/ > > -#define darray_append_string(arr, str) do {const char *__str = (str); darray_append_items(arr, __str, strlen(__str)+1); (arr).size--;} while(0) > +#define darray_append_string(arr, str) do {const char *str_ = (str); darray_append_items(arr, str_, strlen(str_)+1); (arr).size--;} while(0) > #define darray_append_lit(arr, stringLiteral) do {darray_append_items(arr, stringLiteral, sizeof(stringLiteral)); (arr).size--;} while(0) > > #define darray_prepend_string(arr, str) do { \ > - const char *__str = (str); \ > - darray_prepend_items_nullterminate(arr, __str, strlen(__str)); \ > + const char *str_ = (str); \ > + darray_prepend_items_nullterminate(arr, str_, strlen(str_)); \ > } while(0) > #define darray_prepend_lit(arr, stringLiteral) \ > darray_prepend_items_nullterminate(arr, stringLiteral, sizeof(stringLiteral) - 1) > > -#define darray_from_string(arr, str) do {const char *__str = (str); darray_from_items(arr, __str, strlen(__str)+1); (arr).size--;} while(0) > +#define darray_from_string(arr, str) do {const char *str_ = (str); darray_from_items(arr, str_, strlen(str_)+1); (arr).size--;} while(0) > #define darray_from_lit(arr, stringLiteral) do {darray_from_items(arr, stringLiteral, sizeof(stringLiteral)); (arr).size--;} while(0) > > > @@ -263,11 +263,11 @@ typedef darray(unsigned long) darray_ulong; > > #define darray_resize(arr, newSize) darray_growalloc(arr, (arr).size = (newSize)) > #define darray_resize0(arr, newSize) do { \ > - size_t __oldSize = (arr).size, __newSize = (newSize); \ > - (arr).size = __newSize; \ > - if (__newSize > __oldSize) { \ > - darray_growalloc(arr, __newSize); \ > - memset(&(arr).item[__oldSize], 0, (__newSize - __oldSize) * sizeof(*(arr).item)); \ > + size_t oldSize_ = (arr).size, newSize_ = (newSize); \ > + (arr).size = newSize_; \ > + if (newSize_ > oldSize_) { \ > + darray_growalloc(arr, newSize_); \ > + memset(&(arr).item[oldSize_], 0, (newSize_ - oldSize_) * sizeof(*(arr).item)); \ > } \ > } while(0) > > @@ -275,9 +275,9 @@ typedef darray(unsigned long) darray_ulong; > (arr).item = realloc((arr).item, ((arr).alloc = (newAlloc)) * sizeof(*(arr).item)); \ > } while(0) > #define darray_growalloc(arr, need) do { \ > - size_t __need = (need); \ > - if (__need > (arr).alloc) \ > - darray_realloc(arr, darray_next_alloc((arr).alloc, __need)); \ > + size_t need_ = (need); \ > + if (need_ > (arr).alloc) \ > + darray_realloc(arr, darray_next_alloc((arr).alloc, need_)); \ > } while(0) > > #if HAVE_STATEMENT_EXPR==1 -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-08-29 10:08 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-26 16:26 [PATCH 1/2] darray: Fix bug in the darray_remove() macro Damien Grassart 2017-08-27 2:56 ` David Gibson 2017-08-27 21:02 ` Damien Grassart 2017-08-27 21:26 ` [PATCH 1/2] darray: Rename identifiers starting with an underscore Damien Grassart 2017-08-27 21:26 ` [PATCH 2/2] darray: Fix bug in the darray_remove() macro Damien Grassart 2017-08-28 2:48 ` David Gibson 2017-08-28 5:03 ` Damien Grassart 2017-08-28 5:09 ` [PATCH] " Damien Grassart 2017-08-29 4:54 ` David Gibson 2017-08-29 10:08 ` Damien Grassart 2017-08-28 2:43 ` [PATCH 1/2] darray: Rename identifiers starting with an underscore David Gibson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).