 |
C++Talk.NET
C++ language newsgroups
|
| View previous topic :: View next topic |
| Author |
Message |
Guest
|
 Posted: Sat Jul 08, 2006 4:58 am Post subject: What is the output of this program? |
 |
|
//...
string ToUpper(const string& s1)
{
string s2;
for (int i = 0; i < s1.size(); ++i)
if (isalpha(s1[i])) s2[i] = toupper(s1[i]);
return s2;
}
int main()
{
string v1[5] = {"This", "is", "a", "small", "quizz"};
string v2[5];
for (int i = 0; i < 5; ++i) v2[i] = ToUpper(v1[i]);
//..Print out v2. What is the output?
return 0;
}
A mistake made me stare at the screen for a while ...
Could you give some hints to avoid similar mistakes systematically?
Thanks.
Ng.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
 |
Ulrich Eckhardt
Guest
|
| Posted: Sun Jul 09, 2006 4:24 am Post subject: Re: What is the output of this program? |
|
|
dnguyenk (AT) yahoo (DOT) com wrote:
[invalid access into a container]
| Quote: | Could you give some hints to avoid similar mistakes systematically?
|
Use a checked standardlibrary implementation, i.e. one that does (optional)
runtime checks that are not required by the standard itself. STLport for
example would have caught that error.
Uli
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
chandu
Guest
|
| Posted: Sun Jul 09, 2006 4:29 am Post subject: Re: What is the output of this program? |
|
|
| Quote: | string ToUpper(const string& s1)
{
string s2;
for (int i = 0; i < s1.size(); ++i)
if (isalpha(s1[i]))
{ |
//s2[i] = toupper(s1[i]); the size of s2 is not
known so the accessing thru index on it is not appropriate and the
result wud be implementation defined , so either initialize s2 to empty
string by saying this: string s2("", s1.size()); or use the code below:
s2.push_back(toupper(s1[i])); //or
s2.insert(s2.end(), toupper(s1[i]));
}
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
Alf P. Steinbach
Guest
|
| Posted: Sun Jul 09, 2006 4:32 am Post subject: Re: What is the output of this program? |
|
|
* dnguyenk (AT) yahoo (DOT) com:
| Quote: | //...
string ToUpper(const string& s1)
{
|
Class 'string' is undefined. For the sake of discussion, assuming it is
'std::string'. This could have been avoided by not using a 'using'.
Uninformative name. 'result' would have been informative.
| Quote: | for (int i = 0; i < s1.size(); ++i)
|
'int' doesn't necessarily have large enough range.
'int' may produce a warning about signed/unsigned comparision; you
should strive for warning-free compilation, because warnings then tell
you about serious problems.
| Quote: | if (isalpha(s1[i]))
|
'isalpha' (assuming this is 'std::isalpha') may produce an unexpected
result if 'char' is signed. This means Unpredictable Behavior.
Argument should be casted to unsigned.
s2[i] = toupper(s1[i]);
Lack of braces, always use braces for the body of 'if' or a loop.
Assigning to non-existent string element means Undefined Behavior.
'toupper' (assuming this is 'std::toupper') may produce an unexpected
result if 'char' is signed. This means Unpredictable Behavior.
Argument should be casted to unsigned.
| Quote: | return s2;
}
int main()
{
string v1[5] = {"This", "is", "a", "small", "quizz"};
|
Preferably use 'const' for constant data.
| Quote: | string v2[5];
for (int i = 0; i < 5; ++i) v2[i] = ToUpper(v1[i]);
|
Braces.
| Quote: | //..Print out v2. What is the output?
|
Undefined due to at least two reasons (see above).
Unnecessary; this is the default.
No catching of possible exception.
| Quote: | A mistake made me stare at the screen for a while ...
|
Impossible to guess /which/ mistake that was; many to choose from here.
If I had to hazard a guess about which mistake you identified, I think
it was probably not specififying the size of the result string. Do note
that this code has more than one mistake, and also, that I haven't tried
compiling the code: there may be problems I haven't identified above.
| Quote: | Could you give some hints to avoid similar mistakes systematically?
|
1. Don't use 'using' until you gain far more experience
(this e.g. removes doubt about what class a name refers to).
2. Don't use raw indexing [] until you gain far more experience, use
'at' instead
(this catches bugs like forgetting to set the size of the result).
3. Do catch exceptions in 'main', report them on 'std::cerr' and in
that case return EXIT_FAILURE from 'main'
(this helps report e.g. an exception from 'at').
4. RTFM; also use Google to check.
(this helps avoid using e.g. 'toupper' incorrectly).
5. Turn up the warning level of your compiler to max, specify standard
C++, and so on, and make sure you compile with no warnings
(this catches a lot of newbie errors).
6. Do what you did here, have others look at and criticize your code
(if you can).
7. Don't rely on mechanical tools for correctness: your brain is the
absolutely most important tool.
--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
Ron Natalie
Guest
|
| Posted: Sun Jul 09, 2006 4:32 am Post subject: Re: What is the output of this program? |
|
|
dnguyenk (AT) yahoo (DOT) com wrote:
| Quote: | A mistake made me stare at the screen for a while ...
Could you give some hints to avoid similar mistakes systematically?
Thanks.
|
Get a better library implementation? While the standard doesn't
require bounds checking on strings, there's no reason a library
can't provide it. You can have a debug version of the standard
library that provides more rigorous checking than the one you
use in production code.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
Guest
|
| Posted: Sun Jul 09, 2006 4:34 am Post subject: Re: What is the output of this program? |
|
|
| Quote: | A mistake made me stare at the screen for a while ...
Could you give some hints to avoid similar mistakes systematically?
|
Probably the easiest change would be to use the string's "at" function
for range-checked access, and encase your code in a try/catch block to
print an error message.
Also, you could use "vector<string>" instead of "string v1[5]". vector
also provides a at() function, but you usually have to initialize it
with a bunch of .push_back's.
Derek.
Your code would look like this. When executed, it will print a
string::at exception, indicating that you tried to access beyond the
end of a string.
=======
#include <string>
#include <iostream>
using namespace std;
string ToUpper(const string& s1)
{
string s2;
for (int i = 0; i < s1.size(); ++i)
if (isalpha(s1.at(i))) s2.at(i) = toupper(s1.at(i));
return s2;
}
int main()
{
try
{
string v1[5] = {"This", "is", "a", "small", "quizz"};
string v2[5];
for (int i = 0; i < 5; ++i)
{
v2[i] = ToUpper(v1[i]);
cout << v1[i] << " " << v2[i] << endl;
}
}
catch(exception& x)
{
cout << "EXCEPTION: " << x.what() << endl;
}
return 0;
}
=========
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
Heinz Ozwirk
Guest
|
| Posted: Sun Jul 09, 2006 7:27 pm Post subject: Re: What is the output of this program? |
|
|
"chandu" <chandrashekhar.kumar (AT) gmail (DOT) com> schrieb im Newsbeitrag
news:1152397519.073962.299350 (AT) b28g2000cwb (DOT) googlegroups.com...
| Quote: | string ToUpper(const string& s1)
{
string s2;
for (int i = 0; i < s1.size(); ++i)
if (isalpha(s1[i]))
{
//s2[i] = toupper(s1[i]); the size of s2 is not
known
|
The size of s2 is very well known. It is empty, s its size is 0.
Heinz
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
Seungbeom Kim
Guest
|
| Posted: Sun Jul 09, 2006 7:29 pm Post subject: Re: What is the output of this program? |
|
|
derek (AT) antiquark (DOT) com wrote:
| Quote: |
Probably the easiest change would be to use the string's "at" function
for range-checked access, and encase your code in a try/catch block to
print an error message.
Also, you could use "vector<string>" instead of "string v1[5]". vector
also provides a at() function, but you usually have to initialize it
with a bunch of .push_back's.
|
Is .at() intended for debugging? If the OP had known that he/she could
have used .at(), he/she wouldn't have gotten into the problem at first.
And even after the bug is gone, he/she would have to keep suffering the
additional cost of range checking.
My understanding is that .at() is used when a out-of-range exception is
more or less expected, and [] otherwise; and the range checking for
debugging should be done inside the debug operator[] when the debugging
option is turned on. For example, a quality implementation could do
something like the following:
value_type& operator[](size_type i)
{
assert(i < size()); // disabled if NDEBUG is defined
return __internal_data[i];
}
--
Seungbeom Kim
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
jbannon
Guest
|
| Posted: Sun Jul 09, 2006 7:29 pm Post subject: Re: What is the output of this program? |
|
|
dnguyenk (AT) yahoo (DOT) com wrote:
| Quote: | //...
string ToUpper(const string& s1)
{
string s2;
for (int i = 0; i < s1.size(); ++i)
if (isalpha(s1[i])) s2[i] = toupper(s1[i]);
return s2;
}
int main()
{
string v1[5] = {"This", "is", "a", "small", "quizz"};
string v2[5];
for (int i = 0; i < 5; ++i) v2[i] = ToUpper(v1[i]);
//..Print out v2. What is the output?
return 0;
}
A mistake made me stare at the screen for a while ...
Could you give some hints to avoid similar mistakes systematically?
Thanks.
Ng.
SNIP |
There are at least two problems in the code:
1. The statement if (isalpha(s1[i])) s2[i] = toupper(s1[i]); does not
append any non-alphabetic characters to s2. This was the easy bit.
2. This one was a bit harder and it took me a while to see it after a
bit of testing (I am a beginner). String s2 is initialised to the empty
string in the
definition string s2; by std::string's default contructor, thus the
expression s2[i] is invalid for any i > 0. If you do as I did and
replace operator[] with the range-checked version s2.at(i) you will get
an index out of range exception being thrown. What was needed to fix it
is a line like
s2 += islower(s1[i]) ? toupper(s1[i]) : s1[i];
which appends each converted character to s2. The trouble with this of
course is that there are, at least potentially, O(n) allocations + 1
copy (the return
statement). I think we can get around this by calling
s2.reserve(s1.size()) so that we have only 1 allocation.
I'm still wondering why main() uses arrays rather than vectors.
Also, as has already been pointed out, you should not be using int as
the type of the index into the string. The type should be
std::string::size_type either that or use one of the string iterators.
What I ended up with is a loop like this:
for (std::string::size_type i(0); i != s1.size(); ++i)
{
s2 += islower(s1[i]) ? toupper(s1[i]) : s1[i];
}
To shorten this still further, you don't really need the test since
tolower() will return non-alphabetic characters unmodified so we would
simply have s2 += toupper(s1[i]) which has the benefit of not
generating so many temporaries.
You could also avoid calling s1.size() each time round the loop (since
it is invariant) by declaring a constant like
std::string::size_type const s1sz(s1.size());
and then writing the loop as
for (std::string::size_type i(0); i != s1sz; ++i)
....
How to avoid the type of mistake you made? Practice. Code walkthroughs
to see where improvements could be made. Having your code checked by a
partner. Submitting your code to a forum for critique if, like me, you
are a hobbyist and on your own. Check your code against the standard
(the draft is online if you don't have an official copy), though
reading "standardese" is not easy. Get a good tutorial book written in
the modern style (I suggest Accelerated C++ by Andrew Koenig, You Can
Program in C++ by Francis Glassborow and The C++ Programming Language
by Bjarne Stroustrup although the latter isn't wonderful as a tutorial
for beginners but is a good reference)
Hope this helps.
Cheers
Jim..
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
Seungbeom Kim
Guest
|
| Posted: Sun Jul 09, 2006 7:30 pm Post subject: Re: What is the output of this program? |
|
|
Alf P. Steinbach wrote:
| Quote: |
Lack of braces, always use braces for the body of 'if' or a loop.
|
I guess there would exist many people who would not agree with you on
the word 'always'. If it were *always* better to use braces, then the
language would made them mandatory.
--
Seungbeom Kim
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
Frederick Gotham
Guest
|
| Posted: Mon Jul 10, 2006 2:01 am Post subject: Re: What is the output of this program? |
|
|
Seungbeom Kim posted:
| Quote: | Alf P. Steinbach wrote:
Lack of braces, always use braces for the body of 'if' or a loop.
I guess there would exist many people who would not agree with you on
the word 'always'. If it were *always* better to use braces, then the
language would made them mandatory.
|
I remove redundancies where possible.
I don't use unnecessary parentheses, but rather I consult my operator
precedence and associativity table (which I have mostly memorised).
I don't use braces when all I need is one statement. If I've two simple
statments, I'll probably use a comma:
if (*p) ++p, --i;
--
Frederick Gotham
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
Alf P. Steinbach
Guest
|
| Posted: Mon Jul 10, 2006 2:09 am Post subject: Re: What is the output of this program? |
|
|
* Seungbeom Kim:
| Quote: | Alf P. Steinbach wrote:
Lack of braces, always use braces for the body of 'if' or a loop.
I guess there would exist many people who would not agree with you on
the word 'always'. If it were *always* better to use braces, then the
language would made them mandatory.
|
No, the language is not designed for a particular programmer.
--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
James Kanze
Guest
|
| Posted: Mon Jul 10, 2006 2:10 am Post subject: Re: What is the output of this program? |
|
|
dnguyenk (AT) yahoo (DOT) com wrote:
| Quote: | //...
string ToUpper(const string& s1)
{
string s2;
for (int i = 0; i < s1.size(); ++i)
if (isalpha(s1[i])) s2[i] = toupper(s1[i]);
return s2;
}
int main()
{
string v1[5] = {"This", "is", "a", "small", "quizz"};
string v2[5];
for (int i = 0; i < 5; ++i) v2[i] = ToUpper(v1[i]);
//..Print out v2. What is the output?
return 0;
}
A mistake made me stare at the screen for a while ...
Could you give some hints to avoid similar mistakes
systematically?
|
Any decent implementation of the STL, in debug mode, would have
caused a core dump. Which should have made the error pretty
obvious.
Using the usual STL idioms would have avoided the core dump in
your specific case: iterators and the function push_back should
be almost second nature if you're using the STL.
Note too that your samples doesn't begin to test the function.
There's another frequent problem present, and even replacing the
assignment to s2[i] with a push_back won't remove the undefined
behavior. The most usable function toupper is in <locale>, not
<locale.h>, and takes two arguments. A more usual solution for
something this frequent would be a functional object---why the
standard contains a function std::toupper, rather than a class
with the same name, I don't know. At the very least, I'd
extract the std::ctype<char> from the locale once, before the
loop, rather than each time through. Something like:
std::string
toUpper( std::string const& in )
{
std::string result ;
result.reserve( in.size ) ;
typedef std::ctype< char >
CType ;
CType const& ctype
= std::use_facet< CType >( std::locale() ) ;
for ( std::string::const_iterator iter = in.begin() ;
iter != in.end() ;
++ iter ) {
result.push_back( ctype.toupper( *iter ) ) ;
}
return result ;
}
(Of course, this isn't thread safe. To make it thread safe,
you'd need to make a copy of the locale you're using. Another
reason why I prefer the functional object in this case.)
Note too that this really doesn't work in general, because in
many code sets, some (often most) of the lower case characters
are multi-byte encoded. Any reasonable test set will include
things like "Mäße" (which should give "MÄSSE"---or possibly
"MÄSZE" in certain contexts---after toupper). The conversion of
"ß" to "SS" is a bit awkward, no matter what you do, but "ä" to
"Ä" also fails if UTF-8 is being used.
--
James Kanze kanze.james (AT) neuf (DOT) fr
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France +33 (0)1 30 23 00 34
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
James Kanze
Guest
|
| Posted: Mon Jul 10, 2006 2:10 am Post subject: Re: What is the output of this program? |
|
|
jbannon wrote:
| Quote: | dnguyenk (AT) yahoo (DOT) com wrote:
//...
string ToUpper(const string& s1)
{
string s2;
for (int i = 0; i < s1.size(); ++i)
if (isalpha(s1[i])) s2[i] = toupper(s1[i]);
return s2;
}
int main()
{
string v1[5] = {"This", "is", "a", "small", "quizz"};
string v2[5];
for (int i = 0; i < 5; ++i) v2[i] = ToUpper(v1[i]);
//..Print out v2. What is the output?
return 0;
}
A mistake made me stare at the screen for a while ...
Could you give some hints to avoid similar mistakes systematically?
Thanks.
Ng.
SNIP
There are at least two problems in the code:
|
Actually, there's only one major problem: the code is not
idiomatic C++.
| Quote: | 1. The statement if (isalpha(s1[i])) s2[i] = toupper(s1[i]); does not
append any non-alphabetic characters to s2. This was the easy bit.
|
The statement in question doesn't append anything to s2, ever.
That should be an easy bit as well. For the above bit of code
to make sense, he'd have to have initialized s2 with s1. While
the code could then be made to work, I don't think it's really
idiomatic C++. If you had a toupper functional object, the
idiomatic solution would be simply:
std::transform( s1.begin(), s1.end(),
std::back_inserter( s2 ),
toupper() ) ;
Regretfully, the standard doesn't define such an object. (One
could easily argue that this is a design flaw in <locale>.)
IMHO, if you consider this functionality usable, then it is
frequent enough to receive a name, and to get its own functional
object. (That's a big if, however, since there is no way to
make it work with e.g. UTF-8.)
| Quote: | 2. This one was a bit harder and it took me a while to see it
after a bit of testing (I am a beginner).
|
There's no harm in being a beginner, and not knowing something,
but there's one thing that is bothering me: what books are you
using to learn C++, if they don't teach the basics of using a
container. Normally, I would expect a beginner to know how to
use an iterator, and even back_inserter, before he ever
encountered [].
| Quote: | String s2 is initialised to the empty string in the definition
string s2; by std::string's default contructor, thus the
expression s2[i] is invalid for any i > 0.
|
For any i. In the expression s[i], i, converted to a size_t (an
unsigned integral type) must be strictly less than s.size().
| Quote: | If you do as I did and replace operator[] with the
range-checked version s2.at(i) you will get an index out of
range exception being thrown. What was needed to fix it is a
line like
s2 += islower(s1[i]) ? toupper(s1[i]) : s1[i];
|
Given that all of the standard versions of toupper are defined
to return the argument if islower is false, the test is
unnecessary. Given that the one argument version has undefined
behavior when called with a char argument, however, you're still
in trouble.
| Quote: | which appends each converted character to s2. The trouble with
this of course is that there are, at least potentially, O(n)
allocations + 1 copy (the return statement). I think we can
get around this by calling s2.reserve(s1.size()) so that we
have only 1 allocation.
|
You could. Or you could simple suppose that the implementation
wasn't completely stupid, so you don't get an allocation for
every character, and that the number of allocations you do get
won't have an important effect on your program. If it later
turns out that you were wrong, it's no hassle to add the reserve
later. Until then, it's one unnecessary line which has to be
maintained.
| Quote: | I'm still wondering why main() uses arrays rather than vectors.
|
Because main() was around long before std::vector was invented.
--
James Kanze kanze.james (AT) neuf (DOT) fr
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France +33 (0)1 30 23 00 34
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ] |
|
| Back to top |
|
|
James Kanze
Guest
|
| Posted: Mon Jul 10, 2006 2:11 am Post subject: Re: What is the output of this program? |
|
|
Seungbeom Kim wrote:
| Quote: | Alf P. Steinbach wrote:
Lack of braces, always use braces for the body of 'if' or a loop.
I guess there would exist many people who would not agree with
you on the word 'always'. If it were *always* better to use
braces, then the language would made them mandatory.
|
It's always a bad idea to use gets(), but the language allows
it. There's a lot in the language for historical reasons. (I
don't feel as strongly as Alf about always using braces,
although I always use them myself. But the fact that the
language doesn't require it isn't an argument that not doing so
is good programming practice.)
--
James Kanze kanze.james (AT) neuf (DOT) fr
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France +33 (0)1 30 23 00 34
[ 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
