Re: What is the output of this program?
Alf P. Steinbach wrote:
* dnguyenk@yahoo.com:
//...
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'.
string s2;
Uninformative name. 'result' would have been informative.
for (int i = 0; i < s1.size(); ++i)
'int' doesn't necessarily have large enough range.
But realistically, on a 32 bit machine, that won't really be a
problem. (Today. As memory becomes bigger, of course, it could
become reasonable to have strings longer than 2^31 characters.)
'int' may produce a warning about signed/unsigned comparision;
you should strive for warning-free compilation, because
warnings then tell you about serious problems.
That is, IMHO, an important one. Personally, I would have used
int everywhere, including as the return type of size(). But
mixing types in general, and mixing signed and unsigned in
particular, can sometimes have strange results. As a general
rule, stick to the same type as much as possible. In this case,
since you can't very well change the return type of
std::string::size(), you really should be using size_t
through out.
Of course, if you used the more idiomatic iterators, the problem
wouldn't occur to begin with.
if (isalpha(s1[i]))
'isalpha' (assuming this is 'std::isalpha') may produce an unexpected
result if 'char' is signed.
It depends on what headers were included. Generally speaking,
std::isalpha() doesn't have this problem. On the other hand, it
takes two arguments, so it is obviously not the one he is using.
If we assume ::isalpha, from the C header <ctype.h>, then it's
not an "unexpected result", it's undefined behavior. (There's
also a potential thread safety issue, but in practice, it's not
a problem.)
This means Unpredictable Behavior.
According to the standard, undefined behavior. But I agree that
the probability of anything other than just a random result is
pretty low.
Argument should be casted to unsigned.
unsigned char. Casting it to unsigned isn't correct either, as
it can result in values like 0x7FFFFF62.
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.
Same comments as for isalpha, above.
Also, of course, the if isn't necessary. Toupper returns the
argument unchanged if islower is false. (Supposing the problems
with undefined behavior corrected.)
return s2;
}
int main()
{
string v1[5] = {"This", "is", "a", "small", "quizz"};
Preferably use 'const' for constant data.
string v2[5];
for (int i = 0; i < 5; ++i) v2[i] = ToUpper(v1[i]);
Braces.
And a new line. I'd say that that is even more important than
the braces.
//..Print out v2. What is the output?
Undefined due to at least two reasons (see above).
return 0;
Unnecessary; this is the default.
Still, main is a special case; it's much clearer to be explicit.
}
No catching of possible exception.
With all the undefined behavior, he couldn't catch them reliably
anyway:-).
Seriously, I'm curious as to what you mean by this. For a lot
of simple programs, the default behavior of terminating in case
of an exception is quite acceptable. And the only way to catch
all exceptions is a "catch(...)"; what do you do in the catch
block?
A mistake made me stare at the screen for a while ...
Impossible to guess /which/ mistake that was; many to choose from here.
Yes and no. Since he's also shown the set of test data, we know
that it doesn't actually contain values where the signedness of
char can cause problems. (The standard guarantees that all
characters in the basic execution set have positive encodings
when stored in a char.)
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
test compiling the code: there may be problems I haven't identified
above.
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).
Use a good implementation of the STL which will core dump with
an error message in case of error.
Also, prefer iterators to []. It's more idiomatic, and in this
case, when creating a string (or any other collection), the use
of push_back() is also so idiomatic that it should be second
nature.
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').
At least on the systems I work on, NOT catching an unexpected
exception is preferable. It also results in a "failure" (seen
from the OS side), but also gives a core dump which allows for
post mortem analysis; catching the exception prevents this.
--
James Kanze kanze.james@neuf.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! ]