This forum is a read-only archive of the Gammon Software forum (2000–2026). No new posts can be made. Search the archive.

returning local string

Posted by Gohan_TheDragonball on Wed 20 Jun 2007 12:15 PM — 13 posts, 43,981 views.

Gohan_TheDragonball USA #0
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;
}
Zeno USA #1
I think the string you need to return needs to be static.
David Haley USA #2
Yes, otherwise you are returning an address on the stack, which has the potential for getting cleared/etc.
Isthiriel #3
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:
XLIX - XLIX

(Shouldn't that be IL?)

HTH.
David Haley USA #4
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.
Nick Gammon Australia Forum Administrator #5
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

Gohan_TheDragonball USA #6
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;
}
David Haley USA #7
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.
Gohan_TheDragonball USA #8
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.
David Haley USA #9
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.
Gohan_TheDragonball USA #10
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
David Haley USA #11
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.
Nick Gammon Australia Forum Administrator #12
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