 |
C++Talk.NET
C++ language newsgroups
|
| View previous topic :: View next topic |
| Author |
Message |
Lorenzo Bettini
Guest
|
 Posted: Tue Aug 16, 2005 2:39 pm Post subject: question on the comma operator |
 |
|
Hi
I've always used this macro in one of my program
#define foropt for (gengetopt_option_list::iterator it =
gengetopt_options.begin();
it != gengetopt_options.end(), opt = *it;
++it)
like this
gengetopt_option *opt
foropt
{
do something with opt
}
now with g++ (GCC) 4.0.1 (Debian 4.0.1-2)
the foropt loop always segfaults, because it goes one element too much
(thus the opt points to garbage). Why is this? It looks like the
expression "it != gengetopt_options.end()" is ignored...
I fixed the problem by using
it != gengetopt_options.end() && (opt = *it);
but was the previous code wrong?
thanks in advance
Lorenzo
--
+-----------------------------------------------------+
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
 |
Victor Bazarov
Guest
|
| Posted: Tue Aug 16, 2005 6:03 pm Post subject: Re: question on the comma operator |
|
|
Lorenzo Bettini wrote:
| Quote: | I've always used this macro in one of my program
#define foropt for (gengetopt_option_list::iterator it =
gengetopt_options.begin();
it != gengetopt_options.end(), opt = *it;
++it)
like this
gengetopt_option *opt
foropt
{
do something with opt
}
now with g++ (GCC) 4.0.1 (Debian 4.0.1-2)
the foropt loop always segfaults, because it goes one element too much
(thus the opt points to garbage). Why is this? It looks like the
expression "it != gengetopt_options.end()" is ignored...
I fixed the problem by using
it != gengetopt_options.end() && (opt = *it);
but was the previous code wrong?
|
Yes, wrong. The comma operator evaluates its left operand, then has
a sequence point, then evaluates the right operand and the entire
expression has this last value. If you write
it != blah.end(), opt = *it
the expression most likely has undefined behaviour if the 'blah.end()'
iterator is not dereferenceable (which is most likely the case). This is
what happens: the "!=" is evaluated, then that value is _discarded_, then
the assignment is evaluated thus attempting to dereference 'it' which has
_already_ become invalid.
When you write
it != blah.end() && opt = *it
then if the 'not-equal' operator returns 'false', the right-hand side of
the '&&' is _not_ evaluated.
Generally speaking, if you don't intend to check the value of 'opt' as
well, the assignment should be moved to the third part of the 'for'
statement:
for ( ittype it = blah.begin(); it != blah.end(); opt = *it++ )
// ^^^^^^^^^^^
Which will ensure that (a) it only happens if 'it' can be dereferenced
and (b) that the condition does not rely on the 'opt' value for loop
termination. Of course, if 'opt' is a pointer _and_ you want to terminate
your loop not only if you finish the whole collection but also when you
encounter a null pointer among the elements, the '&&' you found should be
retained.
V
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Kyle
Guest
|
| Posted: Tue Aug 16, 2005 11:32 pm Post subject: Re: question on the comma operator |
|
|
Lorenzo Bettini wrote:
| Quote: | Hi
I've always used this macro in one of my program
#define foropt for (gengetopt_option_list::iterator it =
gengetopt_options.begin();
it != gengetopt_options.end(), opt = *it;
++it)
like this
gengetopt_option *opt
foropt
{
do something with opt
}
now with g++ (GCC) 4.0.1 (Debian 4.0.1-2)
the foropt loop always segfaults, because it goes one element too much
(thus the opt points to garbage). Why is this? It looks like the
|
quote from draft
"A pair of expressions separated by a comma is evaluated left to right
and the value of the left expression is discarded. ... The type and
value of the result are the type and value of the right operand"
| Quote: | expression "it != gengetopt_options.end()" is ignored...
I fixed the problem by using
it != gengetopt_options.end() && (opt = *it);
but was the previous code wrong?
thanks in advance
Lorenzo
|
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Thomas Richter
Guest
|
| Posted: Tue Aug 16, 2005 11:33 pm Post subject: Re: question on the comma operator |
|
|
Hi,
| Quote: | #define foropt for (gengetopt_option_list::iterator it =
gengetopt_options.begin();
it != gengetopt_options.end(), opt = *it;
++it)
like this
gengetopt_option *opt
foropt
{
do something with opt
}
now with g++ (GCC) 4.0.1 (Debian 4.0.1-2)
the foropt loop always segfaults, because it goes one element too much
(thus the opt points to garbage). Why is this? It looks like the
expression "it != gengetopt_options.end()" is ignored...
|
That, too:
it != gengetopt_options.end(), opt = *it
The comma operator evaluates its arguments left to right, no
matter what the result of the first statement is, i.e. there is no
"shortcutting" performed as it is done for && or || (&& evalulates
only its second argument if the first is true, || only if the first
is false). Thus, you deference an interator that is not pointing
to a valid object, and thus, the result is undefined. A segfault is
one out of several possible reactions. The final result of the operator,
is then the result of its second expression, that is, the result of
the comparison is, indeed, ignored.
General comment:
1) Some people might argue that you should have a good look at
<algorithms> and should try for_each() to iterate thru a container
like the above.
2) Even though I'm not a friend of for_each(), I would still believe
that making "opt" an argument of the macro would improve readability
a lot - no "hidden" names are required to be known by the reader
to understand the code.
| Quote: | but was the previous code wrong?
|
I afraid so.
So long,
Thomas
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
tony_in_da_uk@yahoo.co.uk
Guest
|
| Posted: Tue Aug 16, 2005 11:34 pm Post subject: Re: question on the comma operator |
|
|
Hi Lorenzo,
Yes, the previous code was wrong. The comma operator performs the !=
test and ignores the result, proceeding to evaluate the assignment.
You'll find that doesn't work as a general idiom, as the types in the
comma-separated expression have to be the same, and it's only
coincidence that the pointer you're assigning can be implicitly
converted to int. The && form benefits from short-circuit evaluation,
is safe, and could be used.
For what it's worth, the code would be better expressed as:
for (gengetopt_option_list::iterator it =
gengetopt_options.begin();
it != gengetopt_options.end(); ++it)
{
gengetopt_option* opt = *it;
// ...
}
It's conventional to put macro names in all uppercase, but for
something simple that you'll only do once per program, better still to
avoid them completely.
Tony
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Thomas Maeder
Guest
|
| Posted: Tue Aug 16, 2005 11:35 pm Post subject: Re: question on the comma operator |
|
|
Lorenzo Bettini <bettini (AT) dsi (DOT) unifi.it> writes:
| Quote: | I've always used this macro in one of my program
#define foropt for (gengetopt_option_list::iterator it =
gengetopt_options.begin();
it != gengetopt_options.end(), opt = *it;
++it)
|
Hmm. Macros are badly behaved beast. I prefer to use them very
reluctantly. And if I define one, I think that it should have a long,
uglyish name so that it stands out - this somewhat compensates for
macros not respecting most scopes.
foropt seems like a particularly bad name for a macro. Especially
since I don't really see what problem it solves.
| Quote: | like this
gengetopt_option *opt
foropt
{
do something with opt
}
now with g++ (GCC) 4.0.1 (Debian 4.0.1-2)
the foropt loop always segfaults, because it goes one element too
much (thus the opt points to garbage). Why is this? It looks like
the expression "it != gengetopt_options.end()" is ignored...
|
That would be a bug.
Unless overloaded, the comma operator first evaluates the
sub-expression on the lefthand side, and then that on the righthand
side. It's value is the value of the sub-expression on the righthand
side.
| Quote: | I fixed the problem by using
it != gengetopt_options.end() && (opt = *it);
|
This will give you compiler errors for most types, because they won't
have the necessary implicit conversion. And if they have it, the
results will be surprising because the loop will stop where you don't
expect it to stop.
Why don't you simply write *it and it-> inside the loop?
| Quote: | but was the previous code wrong?
|
Definitely.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
tony_in_da_uk@yahoo.co.uk
Guest
|
| Posted: Wed Aug 17, 2005 10:00 am Post subject: Re: question on the comma operator |
|
|
| Quote: | for ( ittype it = blah.begin(); it != blah.end(); opt = *it++ )
|
----------------------
That's not right, as opt is only assigned _after_ the first time
through the loop... - Tony
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Martin Bonner
Guest
|
| Posted: Wed Aug 17, 2005 2:35 pm Post subject: Re: question on the comma operator |
|
|
Lorenzo Bettini wrote:
| Quote: | Hi
I've always used this macro in one of my program
#define foropt for (gengetopt_option_list::iterator it =
gengetopt_options.begin();
it != gengetopt_options.end(), opt = *it;
++it)
|
Others have explained what is wrong with this code. For another
approach you might want to consider using BOOST_FOREACH which has been
accepted into Boost, but was just too late to be included in 1.33. The
usage would be something like:
BOOST_FOREACH(value_type opt, gengetopt_options )
{ code of loop }
Note that this is SLIGHTLY different, in that it declares a new
variable 'opt'.
See
http://boost-sandbox.sourceforge.net/libs/foreach/doc/html/index.html
for documentation and the files themselves are at
http://boost-sandbox.sourceforge.net/vault/index.php?directory=eric_niebler
and then download foreach.zip.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Victor Bazarov
Guest
|
| Posted: Wed Aug 17, 2005 2:37 pm Post subject: Re: question on the comma operator |
|
|
[email]tony_in_da_uk (AT) yahoo (DOT) co.uk[/email] wrote:
| Quote: | for ( ittype it = blah.begin(); it != blah.end(); opt = *it++ )
----------------------
That's not right, as opt is only assigned _after_ the first time
through the loop... - Tony
|
That's true. It should be made the first statement of the body of
the loop, then.
V
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Lorenzo Bettini
Guest
|
| Posted: Wed Aug 17, 2005 2:39 pm Post subject: Re: question on the comma operator |
|
|
[email]tony_in_da_uk (AT) yahoo (DOT) co.uk[/email] wrote:
| Quote: | Hi Lorenzo,
Yes, the previous code was wrong. The comma operator performs the !=
test and ignores the result, proceeding to evaluate the assignment.
You'll find that doesn't work as a general idiom, as the types in the
comma-separated expression have to be the same, and it's only
coincidence that the pointer you're assigning can be implicitly
converted to int. The && form benefits from short-circuit evaluation,
is safe, and could be used.
For what it's worth, the code would be better expressed as:
for (gengetopt_option_list::iterat=ADor it =
gengetopt_options.begin();
it != gengetopt_options.end(); ++it)
{
gengetopt_option* opt = *it;
// ...
}
|
I know, but I use this macro in many parts of my program, and I find
this a quicker way
thanks!
Lorenzo
--
+-----------------------------------------------------+
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Lorenzo Bettini
Guest
|
| Posted: Wed Aug 17, 2005 2:40 pm Post subject: Re: question on the comma operator |
|
|
Thomas Richter wrote:
| Quote: | General comment:
1) Some people might argue that you should have a good look at
algorithms> and should try for_each() to iterate thru a container
like the above.
|
I know, but I have to call class methods and I'm not quite confortable
with for_each in this case, especially if you need to pass other
parameters to the method you're calling...
Lorenzo
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Lorenzo Bettini
Guest
|
| Posted: Wed Aug 17, 2005 3:15 pm Post subject: Re: question on the comma operator |
|
|
Thomas Maeder wrote:
| Quote: | I fixed the problem by using
it != gengetopt_options.end() && (opt = *it);
This will give you compiler errors for most types, because they won't
have the necessary implicit conversion. And if they have it, the
results will be surprising because the loop will stop where you don't
expect it to stop.
|
why?
(opt = *it) assigns the content of it (which is a pointer in turn) to
opt and then checks that it is not null...
| Quote: |
Why don't you simply write *it and it-> inside the loop?
|
I use this loop many times, and this way I avoid to write another
instruction...
Lorenzo
--
+-----------------------------------------------------+
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Lorenzo Bettini
Guest
|
| Posted: Wed Aug 17, 2005 3:16 pm Post subject: Re: question on the comma operator |
|
|
Kyle wrote:
| Quote: | quote from draft
"A pair of expressions separated by a comma is evaluated left to right
and the value of the left expression is discarded. ... The type and
value of the result are the type and value of the right operand"
expression "it != gengetopt_options.end()" is ignored...
|
so, probably, up to now, it had always worked because the dereferenced
iterator was NULL...
--
+-----------------------------------------------------+
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Thomas Maeder
Guest
|
| Posted: Wed Aug 17, 2005 11:15 pm Post subject: Re: question on the comma operator |
|
|
Lorenzo Bettini <bettini (AT) dsi (DOT) unifi.it> writes:
| Quote: | I fixed the problem by using
it != gengetopt_options.end() && (opt = *it);
This will give you compiler errors for most types, because they won't
have the necessary implicit conversion. And if they have it, the
results will be surprising because the loop will stop where you don't
expect it to stop.
why?
(opt = *it) assigns the content of it (which is a pointer in turn) to
opt and then checks that it is not null...
|
AFAICT, you didn't tell us that *it evaluates to a pointer.
And even then, I find it awkward to stop the iteration once you hit a
null pointer. Your mileage may vary.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Lorenzo Bettini
Guest
|
| Posted: Fri Aug 19, 2005 3:15 pm Post subject: Re: question on the comma operator |
|
|
Thomas Maeder wrote:
| Quote: | Lorenzo Bettini <bettini (AT) dsi (DOT) unifi.it> writes:
I fixed the problem by using
it != gengetopt_options.end() && (opt = *it);
This will give you compiler errors for most types, because they won't
have the necessary implicit conversion. And if they have it, the
results will be surprising because the loop will stop where you don't
expect it to stop.
why?
(opt = *it) assigns the content of it (which is a pointer in turn) to
opt and then checks that it is not null...
AFAICT, you didn't tell us that *it evaluates to a pointer.
|
sorry, I forgot about that :-)
| Quote: |
And even then, I find it awkward to stop the iteration once you hit a
null pointer. Your mileage may vary.
|
actually, due to the lazy evaluation, the iteration stops when the
iterator reaches the end. Only if it does not reaches the end, it
assigns *it to opt.
The container does not contain null pointers.
Probably I forgot to specify that that loop was not a "general"
iteration: it is a specific case of my program.
Lorenzo
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
|
FAQ
Search
Memberlist
Usergroups
Register
Profile
Log in to check your private messages
Log in
