[WIP] fix:navit:navigation.c:distance_set_last

[charlescurley]

Added a test so that the array subscript is never less than the lower bound. Note: there is no test for the upper bound.

Also, I saw no reason to test for i == 0 right after the code initialized it to zero.

modified:   navit/navigation.c

[metalstrolch]

OK, having time to have a look on what's done in navigation.c I still request the following changes:

1. Mark the distances array const. Used this way it must never ever change.
2. Replace distance_set_last function entirely by replacing it by preprocessor math to get the number of items in distances array. Or at least make it return that. Sizeof may be our friend here, to avoid miscounting.
   If messing with some "cute" code as this, better fix it right than replace it by even "cuter" code.

[charlescurley]

On Mon, 20 Nov 2017 08:18:09 +0000 (UTC) Stefan Wildemann ***@***.***> wrote: metalstrolch requested changes on this pull request. > static int distance_set_last(void) { static int i=0; - if (i == 0) { Attention! Do not remove this, as **i** is a **static** variable and therefore will keep it's value during multiple calls. You can not expect this to be not zero, as it will only be initialized once. Having a static internal variable called **i** is indeed not good style, and somebody should definitly have a look at this to clarify why **i** is static. But one should not remove the check until done so.

It's a mess, and with few comments to tell you what is going on. I know of three reasons why one would declare a variable static. * An interrupt will play with it. * It is a hardware location that will be modified by the hardware, such as a memory mapped I/O port. * To preserve a value across calls to the function in which it is declared. One and two should not apply in a Linux user land program. Some of the things I've seen in commercial operating systems led me to check for either of those two cases. I didn't find any. And neither one would make sense for what the function and its one calling function are supposed to be doing (according to the comments and internal logic). i is used as an index to an array of truncated distances, to be used for voice directions. E.g. "Turn left in 300 meters." instead of "Turn left in 323.28 meters." The possible values to use in the first sentence are in the array distances, also in navigation.c. That array is terminated with -1, clearly not a valid distance. So the sole purpose of the function distance_set_last is to determine the size of the array. Making i static means you only have to do that once. So taking out the if test is harmless, just wasting processor time at run time. Better to leave it in. Cute code. I dislike cute code. It's too easy to misunderstand. In this case, it is also unnecessary. Why size the array at run time at all? We know at compile time the size of the array. Or, rather we can: a #define can specify the size of the array. Use it to initialize the array correctly. Then we can get rid of distance_set_last entirely, with all the associated overhead. Instead of: int distances[]={1,2,3,4,5,10,25,50,75,100,150,200,250,300,400,500,750,-1}; something like: #define SIZE_OF_ARRAY_DISTANCES int distances[SIZE_OF_ARRAY_DISTANCES]={1,2,3,4,5, ... 750}; ... static int round_distance_reduced( int dist ) { int factor = 1; if (dist > distance[SIZE_OF_ARRAY_DISTANCES-1]) { dist=(dist+500)/1000; factor = 1000; } int i=0,d=0,m=0; while (i < SIZE_OF_ARRAY_DISTANCES) { ... In addition to getting rid of a function and speeding up run time ever so slightly, SIZE_OF_ARRAY_DISTANCES also gives you a compile time check for too many initializers. Or, try this: #define SIZE_OF_ARRAY_DISTANCES #define LAST_DISTANCE 750 int distances[SIZE_OF_ARRAY_DISTANCES]={1,2,3,4,5, ... LAST_DISTANCE}; ... static int round_distance_reduced( int dist ) { int factor = 1; if (dist > LAST_DISTANCE) { dist=(dist+500)/1000; factor = 1000; } int i=0,d=0,m=0; while (i < SIZE_OF_ARRAY_DISTANCES) { ... This version removes the necessity of a run time index into the array every time the code calls round_distance_reduced. A small improvement in run time performance. Possibly not worth it because the code is a bit harder to read. Thoughts? Ideas? Brickbats?

…

-- The right of the people to be secure in their persons, houses, papers, and effects, against unreasonable searches and seizures, shall not be violated, and no Warrants shall issue, but upon probable cause, supported by Oath or affirmation, and particularly describing the place to be searched, and the persons or things to be seized. -- U.S. Const. Amendment IV Key fingerprint = CE5C 6645 A45A 64E4 94C0 809C FFF6 4C48 4ECD DFDB

[pgrandin]

@metalstrolch would you like to update your review following @charlescurley 's comment?

[metalstrolch]

OK, having time to have a look on what's done in navigation.c I still request the following changes:

1. Mark the distances array const. Used this way it must never ever change.
2. Replace distance_set_last function entirely by replacing it by preprocessor math to get the number of items in distances array. Or at least make it return that. Sizeof may be our friend here, to avoid miscounting.

If messing with some "cute" code as this, better fix it right than replace it by even "cuter" code.

[charlescurley]

On Tue, 05 Dec 2017 02:34:58 -0800 Stefan Wildemann ***@***.***> wrote: OK, having time to have a look on what's done in navigation.c I still request the following changes: 1. Mark the distances array const. Used this way it must never ever change.

Agree.

2. Replace distance_set_last function entirely by replacing it by preprocessor math to get the number of items in distances array. Or at least make it return that. Sizeof may be our friend here, to avoid miscounting.

Agree. When I worked on this earlier, I wasn't sure sizeof() would work correctly. I looked it up, and it should, with one caveat. The caveat has to do with alignment, on compilers and processors where the size of the data type is less than the word size of the processor. The PDP 11 has a word size of 16 bits. Chars are 8 bits, but align on 16 bit boundaries. An array of chars has one char in each word of memory. So doing this would fail: /* a test program for sizes of arrays. */ #include <stdio.h> char array[] = {1,2,3}; #define SIZE_OF_ARRAY (sizeof (array)/sizeof (char)) #define LAST_ELEMENT (SIZE_OF_ARRAY-1) int main (void) { printf ("Array is size %i.\n", SIZE_OF_ARRAY); printf ("Last Element is %i.\n", array[LAST_ELEMENT]); printf ("Penultimate Element is %i.\n", array[LAST_ELEMENT-1]); return (0); } That works correctly on x86 because the processor will cheerfully pack the chars several to a word. The PDP 11 won't. This is less of a problem with ints because they are usually the word size of the processor: 16 on the PDP 11, 64 on AMD 64 architecture. So: Do we expect to run on any processors where this might be a problem? And do we want to take the risk we might in the future? Is there a better way to do this?

If messing with some "cute" code as this, better fix it right than replace it by even "cuter" code.

Agree. I added code with the above in mind to branch distance_set_last.return.

…

-- The right of the people to be secure in their persons, houses, papers, and effects, against unreasonable searches and seizures, shall not be violated, and no Warrants shall issue, but upon probable cause, supported by Oath or affirmation, and particularly describing the place to be searched, and the persons or things to be seized. -- U.S. Const. Amendment IV Key fingerprint = CE5C 6645 A45A 64E4 94C0 809C FFF6 4C48 4ECD DFDB

[metalstrolch]

This way it looks good. The way the number of elements is defined now seems to be quite common.

I wouldn't care too much about real exotic archs.
But you're right about the PDP11 and maybe the Honeywells of the same time.
Modern DSPs. that sometimes have char with 16 or 32 bit, are not affected by a miscalculation here. It's really only the PDP11.
Btw: shouldn't the PDP11 have 16 bit chars nowadays? Nothing says a char must be equal to 8 bit wide. It just says at least 8 bit as far as I remember.

[charlescurley]

On Wed, 06 Dec 2017 22:42:20 +0000 (UTC) Stefan Wildemann ***@***.***> wrote: This way it looks good. The way the number of elements is defined now seems to be quite common. I wouldn't care too much about real exotic archs.

OK. I think I'll add a comment, just in case.

But you're right about the PDP11 and maybe the Honeywells of the same time. Modern DSPs. that sometimes have char with 16 or 32 bit, are not affected by a miscalculation here. It's really only the PDP11.

OK. And I don't plan on using a PDP-11 for a carputer any time soon. I want to run through this code with gdb just to be sure everything is on the level, and haven't figured out yet how to get it called. So let's not call it good yet.

…

-- The right of the people to be secure in their persons, houses, papers, and effects, against unreasonable searches and seizures, shall not be violated, and no Warrants shall issue, but upon probable cause, supported by Oath or affirmation, and particularly describing the place to be searched, and the persons or things to be seized. -- U.S. Const. Amendment IV Key fingerprint = CE5C 6645 A45A 64E4 94C0 809C FFF6 4C48 4ECD DFDB

[pgrandin]

I want to run through this code with gdb just to be sure everything is on the level, and haven't figured out yet how to get it called. So let's not call it good yet.

I've renamed your PR as a WIP for now, please remove when you feel that it's ready for review.

[sleske]

@charlescurley:
Kudos for thinking thoroughly about the compatibility of your C code.
Just a standard-nitpicking note:

The PDP 11 has a word size of 16 bits. Chars are 8 bits, but align on 16 bit boundaries. An array of chars has one char in each word of memory. So doing this would fail:

char array[] = {1,2,3};
#define SIZE_OF_ARRAY (sizeof (array)/sizeof (char))

While I can't speak for the PDP 11, this cannot happen in standard C (as defined by the ISO/IEC standards, i.e. C99 etc). In standard C, an array is a "contiguously allocated nonempty set of objects", so no padding is allowed between elements. This means that in order for a type to be used in an array, its alignment must divide its size. See for example Can C arrays contain padding in between elements? on stackoverflow.com for details.

tl;dr: We don't need to worry about this, the C standard has our back 📕.

[pgrandin]

so @charlescurley good to go? your PR still says "WIP"

[charlescurley]

On Tue, 09 Jan 2018 10:38:14 -0800 Pierre GRANDIN ***@***.***> wrote: so @charlescurley good to go? your PR still says "WIP"

Meh. I have not had a chance to use gdb on this. Once I do, it will be less than a day, probably an hour's work. Does anyone else think it necessary? Given sleske's discussion, I am not convinced it is necessary.

…

-- A great civilization is not conquered from without until it has destroyed itself within. The essential causes of Rome's decline lay in her people, her morals, her class struggle, her failing trade, her bureaucratic despotism, her stifling taxes, her consuming wars.

-- Will and Ariel Durant, III The Story of Civilization (1944) epilogue

[pgrandin]

Does anyone else think it necessary? Given sleske's discussion, I am not convinced it is necessary.

Yep, I would agree.