Fix implicit conversion to unsigned bugs... #31
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes e.g. negatively strided views work as intended:
OBJ ary = aArray(aInt(1), aInt(2), aInt(3));
OBJ ary_view = aArrayView(ary, aRange(-1, 1, -1));
for ( unsigned ii = 0 ; ii < gsize(ary_view); ii++ ) {
printf (" %u: %i\n", ii, gint(ggetAt(ary_view, aInt(ii))));
won't seg fault now. As usual fully fixing this problem for all pointer
widths is excruciating and inefficient, so we don't. Using ptrdiff_t
gives decent results on 32 bit and works for all realistic offsets on 64
bit. It's tempting to use I64 rather than ptrdiff_t to make the
operation explicit, but that might not work right on 32 bit (I'm not
sure).
This probably isn't all the bugs of this sort. At least the following
look suspicious:
src/Array_alg.c:547:35: warning: conversion from 'long int' to 'U32' {aka 'unsigned int'} may change value [-Wconversion]
src/./tmpl/Vector_alg.c:406:35: warning: conversion from 'long int' to 'U32' {aka 'unsigned int'} may change value [-Wconversion]
src/String_alg.c:308:35: warning: conversion from 'long int' to 'U32' {aka 'unsigned int'} may change value [-Wconversion]
The fixes for those look a little harder though, as the suspicious
intermediate results get sent off to an unsigned argument to
KnuthMorrisPratt() (which function would also likely need to be fixed).
The Range_index() function in Range.h seems to work right and it looks
like the mechanism is well-defined even if it is a little scary. I
added an explicit cast and comment to make it clear what's going on.
I haven't tested all these obviously but they fit the pattern and look safe. Still not done: cases that show up or propagate into qsortFun()/ iqsortSFun()/KnuthMorrisPratt() (statics which show up in multiple places), many in Vector_* because I'm not sure they have the same issue. There are a lot of these. Perhaps Range_index() and Array should return/use I32 and COS be documented to only support Arrays of ~INT32_MAX (maybe -1 or something), which is effectively the case for much of the API anyway. But that's a breaking change to existing simple uses of large arrays.
|
Please let me know if the Array_alg does indeed need fixed and you know how to do it, as I'm interested in gfind/gany so will probably hit those bugs next. |
|
With the second commit gfind and gforeach are working |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes e.g. negatively strided views work as intended:
OBJ ary = aArray(aInt(1), aInt(2), aInt(3));
OBJ ary_view = aArrayView(ary, aRange(-1, 1, -1));
for ( unsigned ii = 0 ; ii < gsize(ary_view); ii++ ) {
printf (" %u: %i\n", ii, gint(ggetAt(ary_view, aInt(ii))));
won't seg fault now. As usual fully fixing this problem for all pointer
widths is excruciating and inefficient, so we don't. Using ptrdiff_t
gives decent results on 32 bit and works for all realistic offsets on 64
bit. It's tempting to use I64 rather than ptrdiff_t to make the
operation explicit, but that might not work right on 32 bit (I'm not
sure).
This probably isn't all the bugs of this sort. At least the following
look suspicious:
src/Array_alg.c:547:35: warning: conversion from 'long int' to 'U32' {aka 'unsigned int'} may change value [-Wconversion]
src/./tmpl/Vector_alg.c:406:35: warning: conversion from 'long int' to 'U32' {aka 'unsigned int'} may change value [-Wconversion]
src/String_alg.c:308:35: warning: conversion from 'long int' to 'U32' {aka 'unsigned int'} may change value [-Wconversion]
The fixes for those look a little harder though, as the suspicious
intermediate results get sent off to an unsigned argument to
KnuthMorrisPratt() (which function would also likely need to be fixed).
The Range_index() function in Range.h seems to work right and it looks
like the mechanism is well-defined even if it is a little scary. I
added an explicit cast and comment to make it clear what's going on.