0

I have the following code:

char stringHour[50], stringMinute[50], stringSecond[50];

// lots of code...
itoa(hour, stringHour, 10);
itoa(minute, stringMinute, 10);
itoa(second, stringSecond, 10);

strcat(":", stringSecond);
strcat(":", stringMinute);
strcat(stringMinute, stringSecond);
// stringHour contains both HH and :SS:MM
strcat(stringHour, stringMinute);
drawText(HRES/2 - 4, VRES - GLYPH_HEIGHT*2, 
stringHour, black);

hour, minute and second are all ints.

What I want to do is diplay time as follows: HH:MM:SS.

The variables are guaranteed to be between 0 and 59 (except hout, 0-24).

drawText is correct.

This block of code crashes my program, and I can't find anything wrong with it. Can you?

Thanks for your time!

F. P.
  • 5,018
  • 10
  • 55
  • 80
  • 1
    For one thing, you are trying to be way too clever. With the calls to `strcat` and `itoa` all nested like that, it's all but impossible to see what's going on. Break them up into multiple statements so that it is clear what you are doing. – James McNellis Jan 01 '11 at 17:18
  • Agreed. Clarified intent with a comment underneath the block. – F. P. Jan 01 '11 at 17:20
  • No, no. A comment is The Wrong Solution to this complexity problem. Make the code less complex by breaking it up into several, easy to comprehend statements. – James McNellis Jan 01 '11 at 17:21
  • I meant I commented my question right here :P But you're right, I'll try that. – F. P. Jan 01 '11 at 17:22

5 Answers5

2

This is wrong:

strcat(":", stringSecond);

You can't use a string literal for the first argument. See strcat doc.

leonbloy
  • 73,180
  • 20
  • 142
  • 190
1

Also, it is best to init arrays to zero.

char stringHour[50] = {0}, stringMinute[50] = {0}, stringSecond[50] = {0};

Then try storing the result of the cat into a string to see what you come up with.

James
  • 1,569
  • 10
  • 18
1

More than likely the bit that is failing is this piece:

... stuff removed
strcat (":", 
... other stuff removed

You almost certainly can't strcat stuff onto a string literal on the platform you are using.

The strcat approach has buffer overflow safety issues as well. Maybe using snprintf might be a better alternative?

clstrfsck
  • 14,715
  • 4
  • 44
  • 59
1

You can't modify string literals; use char buffer[] = "MyLiteral" instead and then strcat() to that.

(The reason behind this fact is that the compiler can optimize code size by consolidating multiple instances of your string literals into one instance in a read-only section of the executable, and so if you could change one instance, you'd change everything else, which would result in unpredictable behavior.)

user541686
  • 205,094
  • 128
  • 528
  • 886
1

Also - your strcat is in reverse order. strcat actually "concatenates" - so change the order. strcat(stringHour, ":"); strcat(stringHour, stringMinute); strcat(stringHour, ":"); strcat(stringHour, stringSecond);

However, this is pretty terrible - why not use sprintf? Also - other libraries except for string building - possibly look into the BSL. The C standard library actually has no error checking for you either - so it's a shoot yourself in the foot, unless you know exactly what you're doing and what is expected from the str* family of functions.

tperk
  • 93
  • 2