«

On 1/13/2013 20:23, glen herrmannsfeldt wrote:
> Shao Miller <> wrote:
>
> (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.

--
- Shao Miller
--
"Thank you for the kind words; those are the kind of words I like to hear.

Cheerily," -- Richard Harter

Shao Miller <> writes:
> On 1/13/2013 19:57, Keith Thompson wrote:
>> Shao Miller <> writes:
>>> 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 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.

> 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 (The_Other_Keith) kst- <http://www.ghoti.net/~kst>
Working, but not speaking, for JetHead Development, Inc.
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

Shao Miller <> writes:
[...]
> 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.

--
Keith Thompson (The_Other_Keith) kst- <http://www.ghoti.net/~kst>
Working, but not speaking, for JetHead Development, Inc.
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

On 1/13/2013 22:08, Keith Thompson wrote:
> Shao Miller <> writes:
>> On 1/13/2013 19:57, Keith Thompson wrote:
>>> Shao Miller <> writes:
>>>> 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.

>> 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.
>

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.

--
- Shao Miller
--
"Thank you for the kind words; those are the kind of words I like to hear.

Cheerily," -- Richard Harter

Shao Miller <> wrote:

(snip, someone wrote)
>> 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.

(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

Shao Miller <> wrote:

(snip, I wrote)

>> 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.

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

On 1/14/2013 16:21, glen herrmannsfeldt wrote:
> Shao Miller <> wrote:
>
> (snip, someone wrote)
>>> 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.
>
> (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.

--
- Shao Miller
--
"Thank you for the kind words; those are the kind of words I like to hear.

Cheerily," -- Richard Harter