views:

267

answers:

4

Given

std::vector<CMyClass> objects;
CMyClass list[MAX_OBJECT_COUNT];

Is it wise to do this?

for(unsigned int i = 0; i < objects.size(); list[i] = objects.at(i++));

Or should I expand my loop to this?

for(unsigned int i = 0; i < objects.size(); i++)
{
  list[i] = objects.at(i);
}
+6  A: 

When in doubt, prefer the form which is easier to understand (expand the loop).

(And I think list[i] = objects.at(i++) leads to undefined behavior.)

KennyTM
it does. I tried.
sum1stolemyname
+11  A: 

The former is undefined behavior. It's not specified whether list[i] is evaluated (to provide an lvalue for the lhs of the assignment) before or after the function call to objects.at.

Hence there is a legal ordering of the various parts of the expression, in which i is accessed (in list[i]) and separately modified (in i++), without an intervening sequence point.

This is precisely the condition for undefined behavior in the C++ standard - whether such a legal ordering exists. IIRC the C standard expresses it slightly differently but with the same effect.

If in doubt, don't write an expression which uses an increment operator, and also uses the same value anywhere else in the expression. You can do it with the comma operator (i++, i++ is fine) and the conditional operator (i ? i++ : i-- is fine) because they have sequence points in them, but it's rarely worth it. || and && likewise, and something like p != end_p && *(p++) = something; isn't totally implausible. Any other use, and if you stare at it long enough you can usually work out an order of evaluation that messes things up.

That's aside from the comprehensibility of complicated for expressions and for loops with empty bodies.

Steve Jessop
jk
Now, using them in the aforementioned for loop would make it quite interesting.
RaphaelSP
-1. Un**defined** behavior? No. Unspecified, certainly. There are two possible orders as you correctly explain, and the standard doesn't require implementations to document which one is used when. But the standard doesn't make everythin unspecified into "Undefined Behavior". That's a much worse case, where anything goes including outright crashes and nasal demons.
MSalters
And to clarify: argument evaluation happens before function calls, function calls are sequence points, so `.at(i++)` has a sequence point after `i++`.
MSalters
@jk: good point, example added.
Steve Jessop
@MSalters: bad point. It is undefined. 5/4 says, "The requirements of this paragraph shall be met for each allowable ordering for the subexpressions of a full expression, otherwise the behavior is __undefined__ " (my emphasis). There is an allowable ordering of the questioner's expression which does not meet the requirement, "the prior value shall be accessed only to determine the value to be stored". For some reason the examples in that paragraph say behavior is unspecified, I guess in those cases the use of `i` alone as the lvalue doesn't access `i` out of turn.
Steve Jessop
"`.at(i++)` has a sequence point after `i++`". is true, but the sequence point only orders the sub-expressions `i++` and `objects` before the use of `objects.at(i++)`. It doesn't order the sub-expression `list[i]` with respect to `i++`, which is what creates the problem. Nasal demons ahoy.
Steve Jessop
Well, for that ordering you'd hit the sequence point of `operator[]()` when evaluating `list[i]`, which is before the evalutation of `objects.at(i++)`. So for that order too you have a sequence point, although a different one. 5/4 merely requires that a sequence point exists for each ordering, not that it's the same sequence point for all possible orderings. But yes, if `list` isn't some kind of list but a plain array, then `list[i]` won't be a function call expression, and you get nasal demons.
MSalters
Even if `[]` is a function call I'm not sure you're saved. The subexpressions could be evaluated first `i++`, then `list[i]`, then `objects.at()`, then `operator=`. This leaves `i++` evaluated and `i` accessed without an intervening sequence point. But I've never been 100% what "prior value" means in the standard, as opposed to if it had said "value". Possibly the "prior value" isn't accessed, but some other value is instead. I think but am not sure that the `i` of `list[i]` is a subexpression (which hence could be evaluated before `i++`, and *would* then access the prior value).
Steve Jessop
Anyway, maybe I'm overly paranoid about this part of the standard, and in some cases I'm asserting undefined behaviour where none exists, because I think some access to `i` is of the "prior value" when in fact it cannot be. I'm willing to be wrong in that direction, rather than attempt any truly hard-core language lawyering to justify (if it can be done) why `i` can't be accessed immediately before `i++` is evaluated :-)
Steve Jessop
+1  A: 

Referencing i in the same expression as i++ is probably undefined behavior. But since it looks like you're using containers, could you maybe write...

list = objects;                               // if they're the same type
list.assign(objects.begin(), objects.end());  // if not
Kristo
Or in the worst case use std::copy if neither of those options is available (say list is a C-array?).
Mark B
A: 

As it has been said already, post-incrementing a variable in the same expression it is used yields undefined behaviour. However, if you wish to keep the compact form, you could introduce a sequence point and go for

for(unsigned int i = 0; i < objects.size(); list[i] = objects.at(i), i++);
RaphaelSP
This looks like the old "every program can be coded as a bodyless for loop".
Gorpik
Doesn't it ? I remember this form to be used quite often in the IOCCC...
RaphaelSP