Do I need to cast the result of strtol to int?

Question

The following code does not give a warning with g++ 4.1.1 and -Wall.

int octalStrToInt(const std::string& s)
{    
    return strtol(s.c_str(), 0, 8);
}

I was expecting a warning because strtol returns a long int but my function is only returning a plain int. Might other compilers emit a warning here? Should I cast the return value to int in this case as a good practice?

Annalisaannalise

Other compilers might, yes. For instance MSVC has warning C4244 when you narrow an integer type. I don't know whether it applies it for long -> int, though, since they're the same size (at least for versions of Windows from this millennium). It's weird though to see a call to strtol without at least 3 lines of error-checking code, so personally I'd add a range check, as Let_Me_Be suggests, or define a range-checking cast-style function. – Carpus

I left all the extra stuff out for simplicity's sake. I'm only interested in whether the cast is needed/recommended. – Annalisaannalise

Aha, with a sufficiently recent GCC and -Wconversion, compiling for e.g. linux/x64, I'd expect a warning. Whether 4.1.1 is sufficiently recent, I don't know. -Wconversion isn't in -Wall or -Wextra, because you get a lot of false positives. – Carpus

Answer 1

You may need the -Wconversion flag to turn these warnings on. However, it won't warn about long -> int, since they are the same size with GCC (the value won't change because of the conversion). But it would if you convert for example long -> short

I suppose simply doing a cast would not be recommended, since that would just cover up the possibility of bugs. It would be OK after you have checked that such a cast won't modify the value to appease the compiler.

Cloyd

Answer 2

Best approach is:

long x = strtol(...); assert(x <= INT_MAX); return (int)x;

You need limits.h and assert.h

Erelia

Check at the negative end, too. – Carpus

Answer 3

If you don't have access to boost::numeric_cast, you can write a simple imitation:

template <typename T, typename S>
T range_check(const S &s) {
    assert(s <= std::numeric_limits<T>::max());
    assert(s >= std::numeric_limits<T>::min());
    return static_cast<T>(s); // explicit conversion, no warnings.
}

return range_check<int>(strtol(some_value,0,8));

Actually that's a bit of a cheat, since it doesn't work for floating point destination types. min() isn't the same bound for them as it is for integer types, you need to check against +/- max(). Exercise for the reader.

Whether you use assert or some other error-handling depends what you actually want to do about invalid input.

There's also boost::lexical_cast (off-hand I don't know how to make that read octal) and stringstream. Read the type you want, not the type that happens to have a C library function for it.

Carpus

Using assert for this feels somewhat wrong. IMO, assert is for catching programmer's errors (e.g the caller should not have passed a NULL pointer), but here the error depends on the runtime data that you give it. It means, the caller should have checked first that the contents of the string are OK for this function, which makes it all a bit useless - now I'd need another function to test first if the conversion would succeed. – Cloyd

Sure, but in example code I tend to use assert unless there's a reason to use a particular other thing. Alternatively I write if (bad_thing) panic();, leaving it to the reader to implement panic(). In real life I agree with you, this function should have a way to indicate failure, perhaps by throwing an exception. Since it doesn't, and I'm reluctant to introduce exceptions, I'm using assert to indicate "there's an error case here. Deal with it." – Carpus

OK then, but I still think this practice is misleading. (For example, can you tell if Let_Me_Be has a similar convention for examples, or is he in earnest recommending assert as the best way to handle bad runtime data?) – Cloyd

I can't, but I don't need to be able to tell, because I'm not grading his code ;-) Kristo says in comments that the error-checking in the real code has been removed to focus on the conversion/cast. If we're going to worry that people will blindly copy the code, then I'd say that passing a null pointer to strtol is an even bigger problem than assert. Neither is an appropriate way to deal with unreliable input data, assuming Kristo's input even is unreliable. – Carpus

Answer 4

You do not see any warning here, because the "int" and "long int" datatypes on your platform have the same size and range. Depending on architecture, they could become different.

To protect yourself from strange errors, use range checking. I suggest you to use std::numeric_limits::min/max (see ).

After range checking you can safely use static_cast or c-style cast.

On the other hand, you can rely on the same functionality implemented in std::stringstream class. The conversion with std::stringstream will be platform-safe and type-safe by default.

Shackelford

Wouldn't see any warning if they were different, either, without specifying -Wconversion. For long -> short on 32 bit x86 you do get a warning on 4.3.2, but not 3.4.4, not sure when the change appeared. – Carpus

Do not sure about -Wconversion, seems it will generate too much information that did not relate to potentialy dangerous type truncation. After 4.3, the -Wp64 was introduced, this flag will detect the type truncation long int->int for sure. – Shackelford

Nice, didn't know about that. I didn't mean the questioner should use -Wconversion, necessarily, just that when you say, "you don't see any warning here because X", it's not just because of X, it's also because the questioner hasn't switched that warning on... – Carpus

Answer 5

Most modern compilers will warn about the conversion and possible truncation depending on the warning level you have configured. On MSVC that is warning level 4.

Best practice would be to return a long from your function, and let the calling code decide how to handle the conversion. Barring that, at least make sure the value you get back from strtol will fit in an int before you return, as suggested by Let_Me_Be.

Teresitateressa