C++ Memory Error when using if(condition) return: Unbelievable

Question

Today I wrote some interesting code that convert an integer to Roman numerals. Whole running codes are here:

#include <iostream>
#include <map>
#include <string>

using namespace std;

string arabic2roman(int i){
//if(i==0) return "ZERO";

map<int, string> m;
m.insert(pair<int,string>(0,"ZERO"));
m.insert(pair<int,string>(1,"I"));
m.insert(pair<int,string>(4,"IV"));
m.insert(pair<int,string>(5,"V"));
m.insert(pair<int,string>(9,"IX"));
m.insert(pair<int,string>(10,"X"));
m.insert(pair<int,string>(40,"XL"));
m.insert(pair<int,string>(50,"L"));
m.insert(pair<int,string>(90,"XC"));
m.insert(pair<int,string>(100,"C"));
m.insert(pair<int,string>(400,"CD"));
m.insert(pair<int,string>(500,"D"));
m.insert(pair<int,string>(900,"CM"));
m.insert(pair<int,string>(1000,"M"));

string roman;
map<int,string>::iterator iter;
for(iter=m.end();iter !=m.begin();iter--){
    while(i >=iter->first){
        roman+=iter->second;
        i-=iter->first;
    }
}
return roman;
}

int main(){
    int test=12345;
    cout << arabic2roman(test) << endl;
    return 0;
}

This code works fine on my Xcode 4.6.2 right now. But if remove "//" in line 8 right before if(i==0) return "ZERO", on Xcode 4.6.2, the program runs endlessly. Can anybody explain this? Thanks!

" It crashed, and the program runs endlessly." which one is it? — Luchian Grigore, Jun 07 '13 at 22:07
I've coded C++ for going on 15 years, and **I'm** the bug-maker, not the language. — John, Jun 07 '13 at 22:10
Because you are de-referencing the `end()` iterator? 3 years of C++ and you don't know you shouldn't do that? :-) — juanchopanza, Jun 07 '13 at 22:10
@John Would vote twice if I could. Add at least +5 years for me ... — πάντα ῥεῖ, Jun 07 '13 at 22:11
There is nothing wrong with the return "ZERO". It should create a temporary, which copied to the return value. The temporary return value, has the lifetime of the expression (just until the ending semicolon, after endl) and should not give any problem. You have a bad compiler. However, this code makes little sense. It does not pay to build a map just to make a single lookup, in general, it does not pay to build an index just for a single lookup, the index construction cost has to be amortized over many lookups. — migle, Jun 07 '13 at 22:11
I mean the whole program. It should output MMMMMMMMMMMMCCCXLV before removing "//", but after removing, the program is unstopped. I run this code on Xcode 4.6.2 — Jun, Jun 07 '13 at 22:15
Hey, hey why do you downvote this question?? Because the OP tries to blame the language, C++ is frustrating sometimes ... — πάντα ῥεῖ, Jun 07 '13 at 22:15
I'm not sure why all of the downvotes on this question? Oh well .... — John, Jun 07 '13 at 22:15
`i` isn't zero, so it doesn't return `"ZERO"` and always goes on to try to execute the loop with the undefined iterator. — Peter Wood, Jun 07 '13 at 22:16
as the program has undefined behavior, anything can happen. With or without the redundant if — Balog Pal, Jun 07 '13 at 22:17
@John, Could not agree more. Good to see many coding magnets here! — Jun, Jun 07 '13 at 22:18
@juanchopanza, sorry I did not get that! Why comment that if-return line, this program works?? — Jun, Jun 07 '13 at 22:20
De-referencing the `end()` iterator is undefined behaviour. In practice this can mean that your program can end up doing all kinds of non-intuitive things, and it is very hard (even pointless) to reason it out. Your best bet is to fix that problem first, then see if it works. — juanchopanza, Jun 07 '13 at 22:22
@BalogPal, please, how will you revise this program to make it robust? — Jun, Jun 07 '13 at 22:22
Just to note, Roman numerals probably weren't used to represent numbers larger than 3999. — Peter Wood, Jun 07 '13 at 22:25
as others already said, make it `for(iter=m.end(),--iter;iter !=m.begin();--iter){` that stops dereferencing end iterator, uncommenting the 0-check, or make it check range — Balog Pal, Jun 07 '13 at 22:30
you deserve it after editing out the original preamble of the question ;-) — Balog Pal, Jun 07 '13 at 22:38
@PeterWood: There are Roman numerals used for "larger" numbers, but it's not commonly used. Here's a reference: http://mathworld.wolfram.com/RomanNumerals.html — Mats Petersson, Jun 07 '13 at 22:56
@BalogPal _'you deserve it after editing out the original preamble of the question'_ That's at least my credit ;-) ... — πάντα ῥεῖ, Jun 07 '13 at 23:20

Manu343726 · Answer 1 · 2013-06-07T22:43:46.580

8

To iterate in reverse order, use rbegin and rend:

for(iter=m.rbegin();iter !=m.rend(); ++iter){
    while(i >=iter->first){
        roman+=iter->second;
        i-=iter->first;
    }
}

http://en.cppreference.com/w/cpp/container/map/rbegin

edited Jun 07 '13 at 22:43

answered Jun 07 '13 at 22:14

Manu343726

13,969
4
40
75

1

+1 good spot and reduce to the real problem – πάντα ῥεῖ Jun 07 '13 at 22:20
oh, its true. I only copy-modify-paste the original code. I know the it++ behavior: http://stackoverflow.com/questions/24901/is-there-a-performance-difference-between-i-and-i-in-c – Manu343726 Jun 07 '13 at 22:43

score 7 · Answer 2 · answered Jun 07 '13 at 22:08

7

for(iter=m.end();iter !=m.begin();iter--){
    while(i >=iter->first){
        roman+=iter->second;
        i-=iter->first;
    }
}

On the first iteration, you dereference m.end() which is illegal. You were unlucky (no, not lucky) it appeared to work with that line commented out, because it was hiding this major bug.

Do it like everyone else, and start from begin(), using iter++.

answered Jun 07 '13 at 22:08

Luchian Grigore

253,575
64
457
625

5

or `rbegin()` to `rend()`. – juanchopanza Jun 07 '13 at 22:11
Jesus! m.end() is a "THEORETICAL" iterator that follows the actual element! No wonder everybody says dereferencing! – Jun Jun 07 '13 at 22:40
Iterating forwards means the algorithm won't work. You have to subtract the largest values first. – Peter Wood Jun 07 '13 at 23:07

C++ Memory Error when using if(condition) return: Unbelievable

2 Answers2