Using for loop iterator after the loop

[goodTweetieBird]

Our code 'guru' says the that in the snippet below the use of n in the
second loop is unsafe but could not say exactly why, something to do
with the value of n depending on how the loop is exited. Maybe it is
compiler dependent but I ran test cases and could not produce
unexpected behavior. Your opinions would be appreciated.

Thanks
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

int i, n;

// Cycle thru arbitration array until an empty slot is found or the
end is reached.
for (n = 0; n < ARB_ARRAY_SIZE; n++)
{
if (arbVal[n].ipAddress == 0)
{
break;
}
arbVal[n].clockQuality = calculateClockQualityMux(arbVal
[n].fpgaStatus);
}

// A modified 'selection sort' routine.
// Does not run if n == 0;
for(i = 0; i < n; i++)
{
...
}

[Victor Bazarov]

Post by goodTweetieBird
Our code 'guru' says the that in the snippet below the use of n in the
second loop is unsafe but could not say exactly why, something to do
with the value of n depending on how the loop is exited. Maybe it is
compiler dependent but I ran test cases and could not produce
unexpected behavior. Your opinions would be appreciated.
Thanks
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
int i, n;

First off, 'i' is not needed here, is it? Declare it where you use it,
not way-way before.

Post by goodTweetieBird
// Cycle thru arbitration array until an empty slot is found or the
end is reached.
for (n = 0; n < ARB_ARRAY_SIZE; n++)
{
if (arbVal[n].ipAddress == 0)
{
break;
}
arbVal[n].clockQuality = calculateClockQualityMux(arbVal
[n].fpgaStatus);
}

'n' can have any value in the range [0,ARB_ARRAY_SIZE], depending on how
the control got here. If 'break' was activated, 'n' does not reach the
"final" value, 'ARB_ARRAY_SIZE'; that can only happen if the loop exits
"normally".

Post by goodTweetieBird
// A modified 'selection sort' routine.
// Does not run if n == 0;
for(i = 0; i < n; i++)

Unless you need 'i' after this loop, declare it inside the 'for':
for (int i = 0; i < n; ++i)

As to your question, nothing is "unsafe" about using 'n' here - its
range is well defined. Check the values of 'i' and how 'i' is used. If
you never try to index the same 'arbVal' pointer with the index that
goes out of range (like 'i+1' on the last iteration of this cycle), then
you're OK.

Post by goodTweetieBird
{
...
}

V

[goodTweetieBird]

Apologies Victor.

Regarding location of declaration of i, I should have stated this is a
C application, not C++. I know this is a C++ list but this list has
the most bestest answerers.

Thanks, all.

[Victor Bazarov]

Post by goodTweetieBird
Apologies Victor.

No need.

Post by goodTweetieBird
Regarding location of declaration of i, I should have stated this is a
C application, not C++. I know this is a C++ list

It's not really. It's both (since both languages supported by the
compiler, except that C is the C90 variation). I assumed (from
experience that mostly questions are about C++) *incorrectly* what
language you were using. My fault.

Post by goodTweetieBird
but this list has
the most bestest answerers.

V

[Ben Voigt [C++ MVP]]

Post by goodTweetieBird
Our code 'guru' says the that in the snippet below the use of n in the
second loop is unsafe but could not say exactly why, something to do
with the value of n depending on how the loop is exited. Maybe it is
compiler dependent but I ran test cases and could not produce
unexpected behavior. Your opinions would be appreciated.

The value of n does depend on the way the loop exits, but that doesn't make
the code unsafe. In fact, searching for an empty slot in an array would
usually be done exactly the way you show.

The only problem I see is that when the loop doesn't break early, your
insertion sort will fail because there's no free space for a new element.

Consider swapping the two exit conditions so that the loop condition is the
one you expect to find first, and the other condition, presumably
representing an error, actually does some error processing:

// find first free entry
// adjust quality for items traversed along the way
int n = 0;
while (arbVal[n].ipAddress != 0) {
arbVal[n].clockQuality = calculateClockQualityMux(arbVal[n].fpgaStatus);
n++;
if (n >= ARB_ARRAY_SIZE) throw "Array full, cannot insert";
}
// insertion sort
for( int i = 0; i < n; i++ ) ...

Of course you should replace the throw with whatever error-handling scheme
is used in your project.

Oh, it's selection sort, not insertion sort, so maybe being full isn't an
error. Adjust accordingly.

Post by goodTweetieBird
Thanks
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
int i, n;
// Cycle thru arbitration array until an empty slot is found or the
end is reached.
for (n = 0; n < ARB_ARRAY_SIZE; n++)
{
if (arbVal[n].ipAddress == 0)
{
break;
}
arbVal[n].clockQuality = calculateClockQualityMux(arbVal
[n].fpgaStatus);
}
// A modified 'selection sort' routine.
// Does not run if n == 0;
for(i = 0; i < n; i++)
{
...
}

[David Webber]

Post by goodTweetieBird
...
for (n = 0; n < ARB_ARRAY_SIZE; n++)
{
if (arbVal[n].ipAddress == 0)
{
break;
}
arbVal[n].clockQuality = calculateClockQualityMux(arbVal
[n].fpgaStatus);
}
...

As others have commented, your code, as it stands, is basically OK.

What would have been disastrous is if you had had, after that loop,
something involving

arbVal[n]

as n might then be ARB_ARRAY_SIZE and out of the legal range of the array
index.

The probability that sooner or later you'll do something like that (which
is innocent enough *inside* the loop) is what makes it "unsafe".

Dave

[goodTweetieBird]

Post by David Webber

Post by goodTweetieBird
...
for (n = 0; n < ARB_ARRAY_SIZE; n++)
{
if (arbVal[n].ipAddress == 0)
{
break;
}
arbVal[n].clockQuality = calculateClockQualityMux(arbVal
[n].fpgaStatus);
}
...

As others have commented, your code, as it stands, is basically OK.
What would have been disastrous is if you had had, after that loop,
something involving
arbVal[n]
as n might then be ARB_ARRAY_SIZE and out of the legal range of the array
index.
The probability that sooner or later you'll do something like that  (which
is innocent enough *inside* the loop)  is what makes it "unsafe".
Dave
--
David Webber
Author of 'Mozart the Music Processor'http://www.mozart.co.uk
For discussion/support seehttp://www.mozart.co.uk/mozartists/mailinglist.htm

Sorry for the apparent potential unintended consequences. What I did
not make clear was that the second for loop covers the whole rest of
the function. In my attempt to not be verbose I left the door open for
other possibles.