[MUDFLAP] Is sizeof(ARRAY[0]) equivalent to sizeof(*ARRAY) ?

[Shao Miller]

(snip)

for (i = 0; i < sizeof arr / sizeof arr[0]; i += 10)

(snip)

Let's assume that we really want to deal only with every 10th element
rather than second-guessing the intent.

That is just what I've been trying to suggest: The diagnostic can be
considered useful for second-guessing the intention. I cannot argue
that if I begin by agreeing that it's false.

Yes, but so can a few million other things.

It doesn't help much to put on each statement:

"Warning: this statement could be wrong"

If you find the statement:

i += 3;

You could have the warning: "Are you sure you meant 3, and not 4?"

and once in about 1e12 times you might be right.

Warnings have a cost, if nothing else than in the time the programmer
has to read them and ignore them. But even more, too many warnings will
make it more likely to miss the ones that are actually important.
The programmer will just give up and turn them off.

So, really really think about how useful a warning will be before
suggesting that it should be added.

Sure. Let me try to provide some of the missing context:

Once upon a time, I saw a warning about a 'for' loop. It turned out
that there was nothing wrong with the 'for' loop, behaviour-wise, but
the loop could've been reworked so as to avoid the warning, and to look
more sensible besides that. We were using -Werror, so the warning
actually stopped the build process, which was undesirable. It was
especially undesirable since it was a false positive for out-of-bounds.
It was, however, still useful for drawing attention to the weird loop,
and for understanding that behind the scenes, the array index and a
pointer arithmetic alternative happened to be translating the same way.

It turns out that we didn't desire such false positives, so we removed
this ambitious form of warning and left the loop as-is.

So I wouldn't be surprised if mudflap did the same thing, even though
the program is fine.

 

[Keith Thompson]

On 1/13/2013 18:39, Keith Thompson wrote: [...]
So you'd rather iterate over every element of the array, and then
test i%10 to decide whether to process the current one?

for (i = 0; i < sizeof arr / sizeof arr[0]; i ++) {
if (i % 10 == 0) {
/* ... */
}
}

I find that more verbose, less clear, and almost certainly less
efficient (unless the compiler is clever enough to transform it
into the first form). All to solve a problem that is not a problem
at all.

If there's some concern about i overflowing the upper limit of its
type (INT_MAX or SIZE_MAX), I'd probably precompute the last index
I want to look at and use that to control the loop. But I see no
point in doing so just to avoid letting i exceed the upper bound
of the array, when the code explicitly avoids accessing anything
outside the array bounds.

No, I was talking about if the loop was for initialization. In such a
case, I'd initialize every element (advancing by 1), but if something
special was needed for every tenth, I could determine that with your 'if'.

Who says the loop is for initialization? It might not even be writing
to the array.

I did. "The loop body for this could very well initialize ten elements
at a time, but I'd probably advance 'i' by 1 instead of by 10, and use
the '% 10' operator in the loop's body, if need be." (If you'll forgive
the 10 with "operator") I'm saying "if, then", not making a claim about
the loop's purpose.

My point is that you were making an unwarranted assumption. The example
illustrates iterating over every 10 (or N) elements of an array.

Absurd.

A function that, for whatever reason, needs to access every 10th
(or every Nth) element, can perfectly sensibly work on an array of
any size. If the array has 10 or fewer elements, the function just
accesses element 0. I see no reason to treat that as a special case.

If you are talking about:

void some_func1(void) {
int arr[4];
int i;

for (i = 0; i < sizeof arr / sizeof arr[0]; i += 10)
/* Anything */
}

then I'd certainly say it's silly! We can see right away that the
advancement of 'i' is silly! Consider what kind of code review you'd
offer someone about this.

It's common to use "magic numbers" (4 and 10 in this case) for code
snippets meant to illustrate a point. In real life, clearly the
value would be either a named constant or a value computed at run
time (in the latter case allocating the array is more complicated).

Is that the trivial point you've been trying to make all this time?

Ok, suppose we replace 4 by ARRAY_SIZE, a macro whose value
is determined by some build-time configuration process. Maybe
ARRAY_SIZE happens to be 4.

Or suppose it's a constant value 95, which will result in the final
value of i being 100, past the end of the array.

If you are talking about:

void some_func1(int * arr, size_t n) {
int i;

for (i = 0; i < n; i += 10)
/* Anything */
}

then I'd certainly say it's not silly at all!

What I *thought* you were objecting to is having i end up with a
value past the last valid index of arr, to the point where you'd
want a compiler to warn you about it. Both code snippets quoted
above have the same non-problem.

 

[Keith Thompson]

Once upon a time, I saw a warning about a 'for' loop. It turned out
that there was nothing wrong with the 'for' loop, behaviour-wise, but
the loop could've been reworked so as to avoid the warning, and to look
more sensible besides that. We were using -Werror, so the warning
actually stopped the build process, which was undesirable. It was
especially undesirable since it was a false positive for out-of-bounds.
It was, however, still useful for drawing attention to the weird loop,
and for understanding that behind the scenes, the array index and a
pointer arithmetic alternative happened to be translating the same way.

I have yet to see, in this thread, an example of a loop that I
would consider "weird" in the way you describe. I wouldn't be
at all surprised to see an example that meets your description,
and I'm mildly curious what the perceived problem was.

It turns out that we didn't desire such false positives, so we removed
this ambitious form of warning and left the loop as-is.

So I wouldn't be surprised if mudflap did the same thing, even though
the program is fine.

I'd be very surprised if mudflap complained about the specific case
you've been talking about, an integer that exceeds the bounds of an
array with code that guarantees it won't be used to index outside the
array.

 

[Shao Miller]

On 1/13/2013 18:39, Keith Thompson wrote:
[...]
So you'd rather iterate over every element of the array, and then
test i%10 to decide whether to process the current one?

for (i = 0; i < sizeof arr / sizeof arr[0]; i ++) {
if (i % 10 == 0) {
/* ... */
}
}

I find that more verbose, less clear, and almost certainly less
efficient (unless the compiler is clever enough to transform it
into the first form). All to solve a problem that is not a problem
at all.

If there's some concern about i overflowing the upper limit of its
type (INT_MAX or SIZE_MAX), I'd probably precompute the last index
I want to look at and use that to control the loop. But I see no
point in doing so just to avoid letting i exceed the upper bound
of the array, when the code explicitly avoids accessing anything
outside the array bounds.

No, I was talking about if the loop was for initialization. In such a
case, I'd initialize every element (advancing by 1), but if something
special was needed for every tenth, I could determine that with your 'if'.

Who says the loop is for initialization? It might not even be writing
to the array.

I did. "The loop body for this could very well initialize ten elements
at a time, but I'd probably advance 'i' by 1 instead of by 10, and use
the '% 10' operator in the loop's body, if need be." (If you'll forgive
the 10 with "operator") I'm saying "if, then", not making a claim about
the loop's purpose.

My point is that you were making an unwarranted assumption. The example
illustrates iterating over every 10 (or N) elements of an array.

For the second time, I'm saying "if, then". I'm handling a
possibility with a response.

Why didn't you suggest that the 'for' loop has nothing to do with
iterating over the array? Maybe the programmer simply wants to load a
value into 'i' and knows something about the length of the array that
can help them to get the right value in there, when combined with the
'for' loop's expression-3?

For just _working_ with an already-initialized array, I'd probably use
precisely this loop (though I might possibly be more inclined to put it
in a function and pass the count). BUT, as was snipped, if the count of
'arr' is 4, then the programmer _really_ ought to have their head
checked.

Absurd.

A function that, for whatever reason, needs to access every 10th
(or every Nth) element, can perfectly sensibly work on an array of
any size. If the array has 10 or fewer elements, the function just
accesses element 0. I see no reason to treat that as a special case.

If you are talking about:

void some_func1(void) {
int arr[4];
int i;

for (i = 0; i < sizeof arr / sizeof arr[0]; i += 10)
/* Anything */
}

then I'd certainly say it's silly! We can see right away that the
advancement of 'i' is silly! Consider what kind of code review you'd
offer someone about this.

It's common to use "magic numbers" (4 and 10 in this case) for code
snippets meant to illustrate a point. In real life, clearly the
value would be either a named constant or a value computed at run
time (in the latter case allocating the array is more complicated).

Is that the trivial point you've been trying to make all this time?

Ok, suppose we replace 4 by ARRAY_SIZE, a macro whose value
is determined by some build-time configuration process. Maybe
ARRAY_SIZE happens to be 4.

Or suppose it's a constant value 95, which will result in the final
value of i being 100, past the end of the array.

Yes. Using 'ARRAY_SIZE' with that loop strikes me as waiting for an
accident.

Some day, someone else could look at the loop and think, "Oh, it seems
that this array is always going to be a multiple of ten, in length." I
don't think a quick glance would make that an unfair assumption.

Also bad: Someone _worries_ about it being out-of-bounds and wastes time
looking at it, because it's suspicious and they recently had a crash.

What I *thought* you were objecting to is having i end up with a
value past the last valid index of arr, to the point where you'd
want a compiler to warn you about it. Both code snippets quoted
above have the same non-problem.

There is another C-devoted forum that I spend some time in. I see a lot
of code flow by. When I see weird loops, I usually mention them to the
programmer. Some day, maybe a compiler could take care of that bit, as
a quick pass.

 

[glen herrmannsfeldt]

(snip, someone wrote)

I did. "The loop body for this could very well initialize ten elements
at a time, but I'd probably advance 'i' by 1 instead of by 10, and use
the '% 10' operator in the loop's body, if need be." (If you'll forgive
the 10 with "operator") I'm saying "if, then", not making a claim about
the loop's purpose.
(snip)

If you are talking about:

void some_func1(void) {
int arr[4];
int i;

for (i = 0; i < sizeof arr / sizeof arr[0]; i += 10)
/* Anything */
}

then I'd certainly say it's silly! We can see right away that the
advancement of 'i' is silly! Consider what kind of code review you'd
offer someone about this.

OK, how about:

#define N 4
void some_func1(void) {
int arr[N];
int i;

for (i = 0; i < sizeof arr / sizeof arr[0]; i += 10)
/* Anything */
}

Depending on how the preprocessor is implemented, the compiler still
sees 4, but the N could change later.

-- glen

 

[glen herrmannsfeldt]

(snip, I wrote)

Sure. Let me try to provide some of the missing context:

Once upon a time, I saw a warning about a 'for' loop. It turned out
that there was nothing wrong with the 'for' loop, behaviour-wise, but
the loop could've been reworked so as to avoid the warning, and to look
more sensible besides that. We were using -Werror, so the warning
actually stopped the build process, which was undesirable. It was
especially undesirable since it was a false positive for out-of-bounds.
It was, however, still useful for drawing attention to the weird loop,
and for understanding that behind the scenes, the array index and a
pointer arithmetic alternative happened to be translating the same way.

Some years ago, I noticed that when I went to the library to look up
a journal article, I would find another interesting, but unrelated,
article at the same time.

It is certainly possible that if you issue enough warnings that some
fraction of them will be near actual errors that the compiler didn't
detect.

Depending on your defintion of "near", you might even make a
statistical case for the warnings.

Consider a compiler that, on every expression evaluation gives a
warning:

"Possible overflow in this expression."

Every once in a while, it might help you notice that you had missed
considering the possible overflow in that expression, but way more
often it just makes you mad.

It turns out that we didn't desire such false positives, so we
removed this ambitious form of warning and left the loop as-is.

If the warning can have false positives (and if it can't, then it
should probably be an error) you have to consider the cost of
those false positives.

Note that the same is true for medical tests, and there is much
discussion on the value of such tests, and when they should and
should not be done.

Balance the cost, including the cost of false positives, against
the benefit, and decide what is worth doing.

-- glen

 

[Shao Miller]

(snip, someone wrote)

I did. "The loop body for this could very well initialize ten elements
at a time, but I'd probably advance 'i' by 1 instead of by 10, and use
the '% 10' operator in the loop's body, if need be." (If you'll forgive
the 10 with "operator") I'm saying "if, then", not making a claim about
the loop's purpose.
(snip)

If you are talking about:

void some_func1(void) {
int arr[4];
int i;

for (i = 0; i < sizeof arr / sizeof arr[0]; i += 10)
/* Anything */
}

then I'd certainly say it's silly! We can see right away that the
advancement of 'i' is silly! Consider what kind of code review you'd
offer someone about this.

OK, how about:

#define N 4
void some_func1(void) {
int arr[N];
int i;

for (i = 0; i < sizeof arr / sizeof arr[0]; i += 10)
/* Anything */
}

Depending on how the preprocessor is implemented, the compiler still
sees 4, but the N could change later.

I'm not too sure how to respond to this.

Yes, if I read this code, I would ask the programmer why they were
incrementing 'i' by 10, but comparing with 4 in a loop.

Yes, if I recall my experience correctly, GCC with some particular
translation options once emitted an "index out-of-bounds"-style warning
when it looked like the index might index beyond the bounds of the array.

No, I don't think there's anything undefined about the example, other
than the missing statement which has been obviously replaced with a
comment for demonstration purposes.