views:

149

answers:

5

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?

+3  A: 

Best approach is:

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

You need limits.h and assert.h

Let_Me_Be
Check at the negative end, too.
Steve Jessop
@Steve OMG, such a stupid mistake
Let_Me_Be
A: 

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.

John Dibling
+2  A: 

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.

Steve Jessop
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.
UncleBens
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."
Steve Jessop
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?)
UncleBens
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.
Steve Jessop
+1  A: 

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.

UncleBens
+1  A: 

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.

+1 for stringstream
UncleBens
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.
Steve Jessop
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.
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...
Steve Jessop