Violating Sequence Point?

Frederick Gotham

Here's a sample function which converts a string to all uppercase:

#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p = std::toupper( *p ), *p++ );
}
Would the "Sequence point rule" be violated if the code were changed to the
following:
#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p++ = std::toupper(*p) );
}
--

Frederick Gotham

Jul 5 '06 #1

Kai-Uwe Bux

Frederick Gotham wrote:

Here's a sample function which converts a string to all uppercase:

#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p = std::toupper( *p ), *p++ );
}
Would the "Sequence point rule" be violated if the code were changed to
the following:
#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p++ = std::toupper(*p) );
}

a) Feels like it: you dereference the same pointer twice within an
expression that, as a side effect, changes the pointer. Sounds dangerous.

Now, suppose that carefull analysis of the C++ standard showed the code in
question to be legit. Would you then consider it good code? Would you be
inclined to put that analysis as a comment next to the code so that a
maintainer does not feel the need to check carefully?

b) Why not avoid borderline cases and comma operator trickery altogether.
Why not put every action on a line of its own:

#include <cctype>

void StringUp( char * p ) {
while ( *p != 0 ) {
*p = std::toupper( *p );
++ p;
}
}

c) I do not understand the assert( *p >= 0 ) in your code. If char is
unsigned, it will never fail, if char is signed, it may. However there is
no reason why StringUp() should not be called on strings containing
negative characters. I cannot see a reason to make that restriction part of
the contract.
Best

Kai-Uwe Bux

Jul 5 '06 #2

Bo Persson

"Kai-Uwe Bux" <jk********@gmx.netskrev i meddelandet
news:e8**********@murdoch.acc.Virginia.EDU...

Frederick Gotham wrote:

>Here's a sample function which converts a string to all uppercase:

#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p = std::toupper( *p ), *p++ );
}
Would the "Sequence point rule" be violated if the code were
changed to
the following:
#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p++ = std::toupper(*p) );
}

a) Feels like it: you dereference the same pointer twice within an
expression that, as a side effect, changes the pointer. Sounds
dangerous.

Now, suppose that carefull analysis of the C++ standard showed the
code in
question to be legit. Would you then consider it good code? Would
you be
inclined to put that analysis as a comment next to the code so that
a
maintainer does not feel the need to check carefully?

b) Why not avoid borderline cases and comma operator trickery
altogether.
Why not put every action on a line of its own:

#include <cctype>

void StringUp( char * p ) {
while ( *p != 0 ) {
*p = std::toupper( *p );
++ p;
}
}

c) I do not understand the assert( *p >= 0 ) in your code. If char
is
unsigned, it will never fail, if char is signed, it may. However
there is
no reason why StringUp() should not be called on strings containing
negative characters. I cannot see a reason to make that restriction
part of
the contract.

It is because std::toupper() takes an int parameter, and doesn't work
for negative values. Simple and transparent code, isn't it? :-)

Also, what about

StringUp("Goodbye World");

What happens?
Bo Persson

Jul 5 '06 #3

Rolf Magnus

Bo Persson wrote:

>>Here's a sample function which converts a string to all uppercase:

#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p = std::toupper( *p ), *p++ );
}
Would the "Sequence point rule" be violated if the code were
changed to
the following:
#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p++ = std::toupper(*p) );
}

a) Feels like it: you dereference the same pointer twice within an
expression that, as a side effect, changes the pointer. Sounds
dangerous.

Now, suppose that carefull analysis of the C++ standard showed the
code in
question to be legit. Would you then consider it good code? Would
you be
inclined to put that analysis as a comment next to the code so that
a
maintainer does not feel the need to check carefully?

b) Why not avoid borderline cases and comma operator trickery
altogether.
Why not put every action on a line of its own:

#include <cctype>

void StringUp( char * p ) {
while ( *p != 0 ) {
*p = std::toupper( *p );
++ p;
}
}

c) I do not understand the assert( *p >= 0 ) in your code. If char
is unsigned, it will never fail, if char is signed, it may. However
there is no reason why StringUp() should not be called on strings
containing negative characters. I cannot see a reason to make that
restriction part of the contract.

It is because std::toupper() takes an int parameter, and doesn't work
for negative values. Simple and transparent code, isn't it? :-)

However, the solution would be to cast the value to unsigned char, not to
assume (assert) that negative value won't ever be there.

Also, what about

StringUp("Goodbye World");

What happens?

Then an old heritage from C jumps in that unfortunately allows this call to
compile. If you're lucky, the compiler gives you a warning.

Jul 5 '06 #4

Kai-Uwe Bux

Bo Persson wrote:

>
"Kai-Uwe Bux" <jk********@gmx.netskrev i meddelandet
news:e8**********@murdoch.acc.Virginia.EDU...

>Frederick Gotham wrote:

>>Here's a sample function which converts a string to all uppercase:

#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p = std::toupper( *p ), *p++ );
}
Would the "Sequence point rule" be violated if the code were
changed to
the following:
#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p++ = std::toupper(*p) );
}

a) Feels like it: you dereference the same pointer twice within an
expression that, as a side effect, changes the pointer. Sounds
dangerous.

Now, suppose that carefull analysis of the C++ standard showed the
code in
question to be legit. Would you then consider it good code? Would
you be
inclined to put that analysis as a comment next to the code so that
a
maintainer does not feel the need to check carefully?

b) Why not avoid borderline cases and comma operator trickery
altogether.
Why not put every action on a line of its own:

#include <cctype>

void StringUp( char * p ) {
while ( *p != 0 ) {
*p = std::toupper( *p );
++ p;
}
}

c) I do not understand the assert( *p >= 0 ) in your code. If char
is
unsigned, it will never fail, if char is signed, it may. However
there is
no reason why StringUp() should not be called on strings containing
negative characters. I cannot see a reason to make that restriction
part of
the contract.

It is because std::toupper() takes an int parameter, and doesn't work
for negative values. Simple and transparent code, isn't it? :-)

Rats, I always confuse the cctype and locale versions.

Also, what about

StringUp("Goodbye World");

What happens?

Good points. So lets try this:

#include <locale>

template < typename CharT >
void StringUp( CharT * p ) {
std::locale loc;
while ( *p != 0 ) {
*p = std::toupper( *p, loc );
++p;
}
}
The templating apparently also fixes the StringUp( "hello world!" ) problem.
Best

Kai-Uwe Bux

Jul 5 '06 #5

Andrew Koenig

"Frederick Gotham" <fg*******@SPAM.comwrote in message
news:cb*******************@news.indigo.ie...

Here's a sample function which converts a string to all uppercase:

#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p = std::toupper( *p ), *p++ );
}

Would the "Sequence point rule" be violated if the code were changed to
the
following:

#include <cassert>
#include <cctype>

void StringUp( char *p )
{
do assert( *p >= 0 );
while( *p++ = std::toupper(*p) );
}

Yes, because there is no sequence point between the left- and right-hand
sides of an assignment. So even though *p must be evaluated before calling
std::toupper (because you can't call a function before you know the values
of its arguments), there is still no defined evaluation order for *p++ and
std::toupper(*p).

It seems clear what you're trying to accomplish: You want to take a pointer
to the initial character of a null-terminated string and make every
character uppercase. But for some reason, you're putting the terminating
null character through toupper as well, even though it makes no sense to do
so.

I would do it this way instead:

while (*p) {
*p = std::toupper(*p);
++p;
}

Alternatively:

for (; *p; ++p)
*p = std::toupper(*p);

because it makes clear the iterative nature of the loop.

Jul 5 '06 #6