CPP for loop through characters of a string starts at unexpected index?

Question

for(unsigned long i=add1.length()-1;i>=0;i--){
        int presum = stoi(to_string(add1.at(i))) + stoi(to_string(add2.at(i))) + curcarry;

I have this code cycling through the characters of a string (add1), starting from its last and going to its first. It then gets the current character using the index and turns it to an integer, adding it to the current character of another string of guaranteed equal length (add2).

I'm getting an string out_of_range error, and upon looking at the debugging menu, I'm seeing that although my strings are both as they should be, with lengths of only 3, the i value is somehow ending up as 18446744073709551615.

Attached is an image of the value of i, alongside the strings whose lengths obviously do not correlate. What's going on?

Debug Values:

john · Answer 1 · 2019-03-06T19:10:57.100

5

For an unsigned value i>=0 is always true. So your index gets to zero and then wraps around to the largest unsigned long value which, in your case, is 18446744073709551615.

Write your loop like this (for instance)

for (unsigned long i = add1.length(); i-- > 0; ) {

edited Mar 06 '19 at 19:10

answered Mar 06 '19 at 19:07

john

85,011
4
57
81

@Aidan Alternative variant: `for(i = length - 1; i < length; --i)` – either variant: don't forget to check initially if length is > 0 at all. – Aconcagua Mar 06 '19 at 19:24
Reverse iterators would also be helpful here. – user4581301 Mar 06 '19 at 19:45

Aconcagua · Answer 2 · 2019-03-07T06:55:52.557

Collecting the comments to john's answer:

As unsigned values are never less than 0, you end up in an endless loop, wrapping around and ending in a huge value after reaching 0.

You have several options now:

Check the index before decrementing (a humoristic view on this see here):
```
for (unsigned long i = add1.length(); i-- > 0; )
```

Profiting from wrapping around behaviour on underflow:

for (unsigned long i = add1.length() - 1; i < add1.length(); --i)

Reverse iterators – I'd consider this the most preferrable variant; but we need to initialise two variables and rely on the sequence (comma) operator, some might disapprove...:

for(auto i1 = add1.rbegin(), i2 = add2.rbegin(); i1 != add1.rend(); ++i1, ++i2)
//                                      ++, even if going reverse!   ^     ^
{
    // use *i1 and *i2 here
}

You might prefer size_t type instead of unsigned long, it will always be of appropriate size and is the type that is returned by std::string::length or any STL container's size function, too.

I do not see, though, where you handle the case of add2 being the shorter string – any of above would end in undefined behaviour then! And if add1 is the shorter one, you start somewhere in the middle of add2. Covering both:

decltype(add1.rend()) ir, er;
for(auto i1 = add1.rbegin(), i2 = add2.rbegin(); ; ++i1, ++i2)
                                                ^ leaving out condition!
{
    if(i1 == add1.rend())
    {
        ir = i2;          // handle rest of add2 after loop
        er = add2.rend(); // needed to be able to compare with appropriate end
        break;
    }
    if(i2 == add2.rend())
    {
        ir = i1;          // handle rest of add1 after loop
        er = add1.rend(); // needed to be able to compare with appropriate end
        break;
    }
    // adding the values as you did before
}
for(; ir != er; ++ir)
{
    // special handling: append whatever remained from add1 or add2
}

Finally: stoi(to_string(addN.at(i))) – I can hardly imagine anything less efficient...

at does range checking, but via your loop, you assured range already (presuming you fixed it according to one of the variants above), so index operator (addN[i]), which doesn't, is better choice. Admitted, this time, as the loop was incorrect, at prevented you from undefined behaviour. So you might start with at first and replace with index operator as soon as you know the code runs correctly...
(Much more heavy): The intermediate strings are a (really) long way round. As C++ standard guarantees for digits (only!!!) having subsequent code points, you can simply do addN[i] - '0' (or *iX - '0', if using iterators) to get the value. If you want to stay portable, don't assume this for letters, too, like in if('A' <= c && c <= 'Z') c += 'a' - 'A' (for this example there already exists a function doing the same, which you absolutely should prefer).

If we are at effiency already: If you prepend the digits to the result string all the new ciphers (well, code is not shown, but I could imagine you doing so...), you'll keep moving all the already inserted values again and again. Rather append the new digits and on completion apply std::reverse to the result.

CPP for loop through characters of a string starts at unexpected index?

2 Answers2