Pointer and regular variable with the same name

Question

In the "Memory and pointers" chapter of the book "Embedded C programming", Mark Siegesmund gives the following example:

void Find_Min_Max( int list[], int count, int * min, int * max){
    for( int i=0, min=255, max=0; i<count; i++){
        if( *min>list[i])
            *min=list[i];
        if( *max<list[i] )
            *max=list[i];
    }
}
// call like this: Find_Min_Max( table, sizeof(table), &lowest, &highest);

If I understand correctly:

table is an array of int,
count is the size of the array
when calling, &lowest and &highest are addresses of int variables where one wants to store the results
int * min and int * max in the function definition refer to pointers * min and * max, both to integer types
the &lowest and &highest as defined in his last line are actually pointers to an address with an int type variable

(Not 100% sure about that last one.)

Inside the for loop, he each time compares the next int in the array list with what is at the pointer *min and *max addresses and updates the values at those addresses if necessary.

But in the definition of the loop, he defines min = 255, max = 0.

It seems to me as if these are two completely new variables that have not been initialized. Should that line not be

for( int i=0, *min=255, *max=0: i<count; i++){

Is that a mistake in the book or something I misunderstand?

Not the only mistake in that code : `:` instead of `;`, hiding the `min` and `max` parameters inside the loop, trying to dereference `int` variables, assuming the list can contain no more than 255 items, not covering empty lists, ... — Sander De Dycker, Feb 07 '20 at 10:30
That : instead of ; is a typo of mine (now corrected). Where is the dereferencing happening? What do you mean with hiding the min/max parameters inside the loop? — Dieter Vansteenwegen ON4DD, Feb 07 '20 at 10:49
@DieterVansteenwegenON4DD : the `for` loop init statement defines 3 new `int` variables (`i`, `min` and `max`). The latter two have the same name as variables from the surrounding scope, so they hide those in the scope of the `for` loop. Ie. when using `min` inside the `for` loop, it refers to the `int` defined in the init statement - when using `min` outside the `for` loop, it refers to the `int*` parameter. — Sander De Dycker, Feb 07 '20 at 11:11
@SanderDeDycker That makes sense and takes away some of my doubts. I can't mark it as the answer since it is in a comment though... — Dieter Vansteenwegen ON4DD, Feb 07 '20 at 11:33

score 3 · Accepted Answer · answered Feb 07 '20 at 12:33

It seems like an error in the book - it does indeed declare new variables inside the loop. (Hint: before publishing programming books, at least compile the code first...)

But even with that embarrassing bug fixed, the code is naively written. Here's more mistakes:

Always const qualify array parameters that aren't modified.
Always use stdint.h in embedded systems.
Never use magic numbers like 255. In this case, use UINT8_MAX instead.

The above is industry standard consensus. (Also required by MISRA-C etc.)

Also, it is most correct practice to use size_t rather than int for size of arrays, but that's more of a style issue remark.

In addition, a better algorithm is to have the pointers point at the min and max values found in the array, meaning we don't just get the values but also their locations in the data container. Finding the location is a very common use-case. It's about the same execution speed but we get more information.

So if we should attempt to rewrite this to some book-worthy code, it would rather look like:

void find_min_max (const uint8_t* data, size_t size, const uint8_t** min, const uint8_t** max);

Bit harder to read and use with pointer-to-pointers, but more powerful.

(Normally we would micro-optimize pointers of same type with restrict, but in this case all pointers may end up pointing at the same object, so it isn't possible.)

Complete example:

#include <stddef.h>
#include <stdint.h>

void find_min_max (const uint8_t* data, size_t size, const uint8_t** min, const uint8_t** max)
{
  *min = data;
  *max = data;

  for(size_t i=0; i<size; i++)
  {
    if(**min > data[i])
    {
      *min = &data[i];
    }
    if(**max < data[i])
    {
      *max = &data[i];
    }
  }
}

Usage example for PC: (please note that int main (void) and stdio.h shouldn't be used in embedded systems.)

#include <stdio.h>
#include <inttypes.h>

int main (void)
{
  const uint8_t data[] = { 1, 2, 3, 4, 5, 4, 3, 2, 1, 0};
  const uint8_t* min;
  const uint8_t* max;

  find_min_max(data, sizeof data, &min, &max);

  printf("Min: %"PRIu8 ", index: %d\n", *min, (int)(min-data));
  printf("Max: %"PRIu8 ", index: %d\n", *max, (int)(max-data));

  return 0;
}

Disassembling this search algorithm for ARM gcc -O3:

find_min_max:
        cmp     r1, #0
        str     r0, [r2]
        str     r0, [r3]
        bxeq    lr
        push    {r4, lr}
        add     r1, r0, r1
.L5:
        mov     lr, r0
        ldr     ip, [r2]
        ldrb    r4, [ip]        @ zero_extendqisi2
        ldrb    ip, [r0], #1    @ zero_extendqisi2
        cmp     r4, ip
        strhi   lr, [r2]
        ldr     r4, [r3]
        ldrbhi  ip, [r0, #-1]       @ zero_extendqisi2
        ldrb    r4, [r4]        @ zero_extendqisi2
        cmp     r4, ip
        strcc   lr, [r3]
        cmp     r1, r0
        bne     .L5
        pop     {r4, pc}

Not the most effective code still, very branch-intensive. I think there's plenty of room for optimizing this further, if the aim is library-quality code. But then it's also a specialized algorithm, finding both min and max, and their respective indices.

For small data sets it would probably be wiser to just sort the data first, then just grab min and max out of the sorted smallest and largest indices. If you plan to search through the data for other purposes elsewhere in the code, then definitely sort it first, so that you can use binary search.

score 1 · Answer 2 · answered Feb 07 '20 at 11:58

int i=0, min=255, max=0 and int i=0, *min=255, *max=0 both define three new variables, which are initialized, but used incorrectly in the loop body.

The limits should be initialized before the loop:

*min=255;
*max=0;
for(int i=0; i<count; i++)

Alternatively the new variable i could be defined before the loop, but this is not as easy to read as the first one:

int i;
for(i=0, *min=255, *max=0; i<count; i++)

Note that if there are values smaller than 0 or larger than 255, the returned minimum and maximum values will be incorrect.

Regarding your note: The safe way to avoid this kind of problem is to initialize both `min` and `max` to the first value, i.e. `list[0]` — tonypdmtr, Feb 07 '20 at 18:02

score 0 · Answer 3 · answered Feb 07 '20 at 11:02

0

It is some mistake in the code. Maybe it should read:

for( int i=0, *min=255, *max=0; i<count; i++){
        if( *min>list[i])
            *min=list[i];
        if( *max<list[i] )
            *max=list[i];
    }

answered Feb 07 '20 at 11:02

Artem Moroz

119
1
5

3

This defines a new variable (`int *min = 255`) and then dereferences it, resulting to access violation. – VLL Feb 07 '20 at 11:06
@vll so the better option would be to initialize *min and *max before the for loop? – Dieter Vansteenwegen ON4DD Feb 07 '20 at 11:35

Pointer and regular variable with the same name

3 Answers3