Notice: Any messages purporting to come from this site telling you that your password has expired, or that you need to verify your details, confirm your email, resolve issues, making threats, or asking for money, are
spam. We do not email users with any such messages. If you have lost your password you can obtain a new one by using the
password reset link.
Due to spam on this forum, all posts now need moderator approval.
Entire forum
➜ SMAUG
➜ SMAUG coding
➜ returning local string
It is now over 60 days since the last post. This thread is closed.
Refresh page
Posted by
| Gohan_TheDragonball
USA (183 posts) Bio
|
Date
| Wed 20 Jun 2007 12:15 PM (UTC) |
Message
| i forgot how to properly do this. i have a variable i am creating locally and returning, but i was given warnings about that, and i forget how to write the code properly to remove this warning.
char *intToRoman( int value )
{
char result[MAX_STRING_LENGTH];
result[0] = '\0';
if ( value <= 0 )
return result;
while ( value > 0 )
{
if ( value >= 1000 )
{
strcat( result, "M" );
value -= 1000;
}
else if ( value >= 900 )
{
strcat( result, "CM" );
value -= 900;
}
else if ( value >= 500 )
{
strcat( result, "D" );
value -= 500;
}
else if ( value >= 400 )
{
strcat( result, "CD" );
value -= 400;
}
else if ( value >= 100 )
{
strcat( result, "C" );
value -= 100;
}
else if ( value >= 90 )
{
strcat( result, "XC" );
value -= 90;
}
else if ( value >= 50 )
{
strcat( result, "L" );
value -= 50;
}
else if ( value >= 40 )
{
strcat( result, "XL" );
value -= 40;
}
else if ( value >= 10 )
{
strcat( result, "X" );
value -= 10;
}
else if ( value >= 9 )
{
strcat( result, "IX" );
value -= 9;
}
else if ( value >= 5 )
{
strcat( result, "V" );
value -= 5;
}
else if ( value >= 4 )
{
strcat( result, "IV" );
value -= 4;
}
else
{
strcat( result, "I" );
value--;
}
}
return result;
}
| Top |
|
Posted by
| Zeno
USA (2,871 posts) Bio
|
Date
| Reply #1 on Wed 20 Jun 2007 12:37 PM (UTC) |
Message
| I think the string you need to return needs to be static. |
Zeno McDohl,
Owner of Bleached InuYasha Galaxy
http://www.biyg.org | Top |
|
Posted by
| David Haley
USA (3,881 posts) Bio
|
Date
| Reply #2 on Wed 20 Jun 2007 05:32 PM (UTC) |
Message
| Yes, otherwise you are returning an address on the stack, which has the potential for getting cleared/etc. |
David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone
http://david.the-haleys.org | Top |
|
Posted by
| Isthiriel
(113 posts) Bio
|
Date
| Reply #3 on Thu 21 Jun 2007 11:38 AM (UTC) |
Message
| Potentially? It is free'd as soon as the function returns so the returned pointer is dangling before it is even used :(
Declaring the retvalue as a static will avoid that problem with the proviso that you do not call the function again before using the result.
For.ex:printf("%s - %s\n", intToRoman(3), intToRoman(49))
will produce:
(Shouldn't that be IL?)
HTH. | Top |
|
Posted by
| David Haley
USA (3,881 posts) Bio
|
Date
| Reply #4 on Thu 21 Jun 2007 05:53 PM (UTC) |
Message
| Well, the stack frame is technically popped, but not cleared: its contents aren't necessarily wiped. So you might still find your string there. Of course, it is a Very Bad Idea to even think about relying on that. |
David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone
http://david.the-haleys.org | Top |
|
Posted by
| Nick Gammon
Australia (23,133 posts) Bio
Forum Administrator |
Date
| Reply #5 on Thu 21 Jun 2007 09:43 PM (UTC) |
Message
|
Quote:
char result[MAX_STRING_LENGTH];
You know how long that is? 4096 bytes. That is 4 Kb.
How big a roman numeral are you expecting here? That is long enough for 51 lines of 80 characters per line.
If you make that static, then the program will always occupy 4 Kb more memory than it used to. I know memory is cheap these days, but even so, I suggest working out how big a buffer is really required.
For example, a simple test on the input number (eg. not to be > 10000), and you can probably work out that the buffer only needs to be 30 bytes or so.
Actually, a quick test shows that for big numbers, the roman numeral generated gets large very quickly. For example, calculating the Roman Numeral of 10000000 generates 10000 bytes (all of them are "M").
So, for big numbers the buffer is too small, and for small ones it is much too large. For a modest number, like 2187, I got the result MMCLXXXVII. This is obviously a lot less than 4096 bytes long.
I suggest a check at the start, to limit the number to be (say) less than 10000, and then make the buffer size much smaller.
Incidentally, this is your routine converted into Lua so I could play with it:
function RomanNumerals (value)
local result = ""
if value <= 0 then
return result
end -- if
while value > 0 do
if value >= 1000 then
result = result .. "M"
value = value - 1000
elseif value >= 900 then
result = result .. "CM"
value = value - 900
elseif value >= 500 then
result = result .. "D"
value = value - 500
elseif value >= 400 then
result = result .. "CD"
value = value - 400
elseif value >= 100 then
result = result .. "C"
value = value - 100
elseif value >= 90 then
result = result .. "XC"
value = value - 90
elseif value >= 50 then
result = result .. "L"
value = value - 50
elseif value >= 40 then
result = result .. "XL"
value = value - 40
elseif value >= 10 then
result = result .. "X"
value = value - 10
elseif value >= 9 then
result = result .. "IX"
value = value - 9
elseif value >= 5 then
result = result .. "V"
value = value - 5
elseif value >= 4 then
result = result .. "IV"
value = value - 4
else
result = result .. "I"
value = value - 1
end -- if
end -- while
return result
end -- function
print (RomanNumerals (19850)) --> MMMMMMMMMMMMMMMMMMMDCCCL
|
- Nick Gammon
www.gammon.com.au, www.mushclient.com | Top |
|
Posted by
| Gohan_TheDragonball
USA (183 posts) Bio
|
Date
| Reply #6 on Fri 22 Jun 2007 12:28 AM (UTC) |
Message
| i appreciate the feedback and suggestions, changing it to static cleared the warning and the function works perfectly.
heres what i got now:
char *intToRoman( int value )
{
static char result[100];
result[0] = '\0';
if ( value <= 0 )
return result;
while ( value > 0 )
{
if ( value >= 1000 )
{
strcat( result, "M" );
value -= 1000;
}
else if ( value >= 900 )
{
strcat( result, "CM" );
value -= 900;
}
else if ( value >= 500 )
{
strcat( result, "D" );
value -= 500;
}
else if ( value >= 400 )
{
strcat( result, "CD" );
value -= 400;
}
else if ( value >= 100 )
{
strcat( result, "C" );
value -= 100;
}
else if ( value >= 90 )
{
strcat( result, "XC" );
value -= 90;
}
else if ( value >= 50 )
{
strcat( result, "L" );
value -= 50;
}
else if ( value >= 40 )
{
strcat( result, "XL" );
value -= 40;
}
else if ( value >= 10 )
{
strcat( result, "X" );
value -= 10;
}
else if ( value >= 9 )
{
strcat( result, "IX" );
value -= 9;
}
else if ( value >= 5 )
{
strcat( result, "V" );
value -= 5;
}
else if ( value >= 4 )
{
strcat( result, "IV" );
value -= 4;
}
else
{
strcat( result, "I" );
value--;
}
}
return result;
}
| Top |
|
Posted by
| David Haley
USA (3,881 posts) Bio
|
Date
| Reply #7 on Fri 22 Jun 2007 12:35 AM (UTC) |
Message
| Your function will at worse crash the game and at best do something bad if you are given a number that causes the numeral to be greater than 100 characters in length. I'd do what Nick suggested and add a cap on the number; if it's greater than a few tens of thousands, you might want to return an empty string or an error or something of the sort.
I imagine that since this is being printed to players you don't expect numerals greater than a few characters anyhow.
Nonetheless, it's better to insert the check: it's extremely easy to do and makes the code more bullet-proof. |
David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone
http://david.the-haleys.org | Top |
|
Posted by
| Gohan_TheDragonball
USA (183 posts) Bio
|
Date
| Reply #8 on Fri 22 Jun 2007 01:02 AM (UTC) |
Message
| yeah guess i should, currently the only place the function is called from is my automated clan headquarters creation system, the only number being injected is the clans number, which just started at 1. i didn't think to put a check in since it would take many a year to reach that high a clan number. thanks for the suggestion though. | Top |
|
Posted by
| David Haley
USA (3,881 posts) Bio
|
Date
| Reply #9 on Fri 22 Jun 2007 01:06 AM (UTC) |
Message
| I guess it's a question of habits. Whenever you are dealing with fixed-size buffers, adding the safety checks is a good habit to get into. Even if for this particular function you are sure there won't be a problem, if you don't force yourself to always do it, you might forget some time when it really matters. And also, maybe later, you will let players make their own roman numerals, and forget about the number range problem, and players might start messing with it and crash the game. |
David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone
http://david.the-haleys.org | Top |
|
Posted by
| Gohan_TheDragonball
USA (183 posts) Bio
|
Date
| Reply #10 on Fri 22 Jun 2007 05:55 AM (UTC) |
Message
| now heres a wierd thing, i had lowered the size of result to 100, and i was testing it out to see how big a number would get me to 100:
static char result[100];
i tried 99,999 and got:
MMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMCMXCIX
now that is 104 characters, shouldn't that have technically caused a crash
| Top |
|
Posted by
| David Haley
USA (3,881 posts) Bio
|
Date
| Reply #11 on Fri 22 Jun 2007 07:18 AM (UTC) |
Message
| That will cause a stack overflow. The string is extending beyond its normal boundaries, overwriting whatever is after it in the data segment of the program. Whatever goes after that static string got corrupted. The crash would occur depending on when and how you access that memory, and what is inside it. Either way, something got broken one way or another. |
David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone
http://david.the-haleys.org | Top |
|
Posted by
| Nick Gammon
Australia (23,133 posts) Bio
Forum Administrator |
Date
| Reply #12 on Fri 22 Jun 2007 12:25 PM (UTC) |
Message
|
Quote:
Your function will at worse crash the game and at best do something bad if you are given a number that causes the numeral to be greater than 100 characters in length.
It is 99 actually, because the null byte at the end takes a character.
Quote:
... now that is 104 characters, shouldn't that have technically caused a crash
If you overflow the stack, even by a single byte, you are setting yourself up for trouble. Maybe your test won't crash. Maybe it won't crash for 5 minutes, or ever. But one day you will find that, a minute after someone looks at the clan number (which by that stage has gone very large for some reason), and then they do something else with that part of memory, it will crash, and you won't connect the two events.
It's really the principle of the thing. As David says, if you design the algorithm thinking "it will never be greater than 1000", then put the test in. Something like this:
if ( value > 1000 )
{
strcpy (result, "too big");
return result;
}
With weird things like Roman Numerals, I would actually write a short test program (like the Lua one), and pump through every possible number you are expecting (eg. 1 to 1000) and get the test to inform you which was the longest string it ever generated. Then make the buffer at least as long as that, plus 1 for the string terminator byte.
For example, using the Lua code above, I added this bit:
max_len = 0
for i = 1, 1000 do
s = RomanNumerals (i)
if #s > max_len then
max_len = #s
max_number = i
max_result = s
end -- if
end -- for
print (string.format ("Largest was %i, length %i, result was %s",
max_number, max_len, max_result))
The result was:
Largest was 888, length 12, result was DCCCLXXXVIII
If you try 10000, the result is:
Largest was 9888, length 21, result was MMMMMMMMMDCCCLXXXVIII
|
- Nick Gammon
www.gammon.com.au, www.mushclient.com | Top |
|
The dates and times for posts above are shown in Universal Co-ordinated Time (UTC).
To show them in your local time you can join the forum, and then set the 'time correction' field in your profile to the number of hours difference between your location and UTC time.
32,866 views.
It is now over 60 days since the last post. This thread is closed.
Refresh page
top