How to cast properly? (Or should I even do it?)

Question

If I have code such as

class CString { int GetLength(); };

bool smaller(CString s1, std::string s2) {
    return s2.size() > s1.GetLength();
}

What is the best thing for me to do?

Change s1.GetLength() to (size_t)c.GetLength()?
This would get help get rid of a compiler warning regarding "signed-unsigned mismatch", and communicate my intention to cast, and is by far the easiest route. But it's probably frowned upon. :(
Change s1.GetLength() to static_cast<size_t>(c.GetLength())?
This would get help get rid of the warning, with "The Correct" kind of cast.
Change s1.GetLength() to static_cast<std::string::size_type>(c.GetLength())?
It's extremely verbose... is there a practical benefit to this abstraction, or should I break it?
Leave it as is?
This would help make the compiler do overflow-checking using the /RTCc switch (my main concern here), at the expense of a warning.
Do something else?
Should I make my own casting function? Use a macro? Should I check at run-time as well as compile-time? Any other ideas?

Edit:

It seems like the example is being taken a little too literally...

I obviously didn't mean to talk just about CString::GetLength(). That particular method is certainly not a huge worry of mine. :) What I am worried about is the more general case, of when I'm getting an integer that's never supposed to be negative, but which could theoretically be, due to bugs.

Heck, I might be writing a method that does this, in order to override another piece of code -- so I can't change the signature. And my code could certainly have bugs, even though I wouldn't expect it.

In such a case, what should I do?

I would definitely stay away from the last one, and probably go with the first. The others seem to verbose for this. — Luchian Grigore, Apr 05 '12 at 08:56
Does it make sense for `GetLength()` to return negative values in this context? — juanchopanza, Apr 05 '12 at 08:58
@hmjd: I can't change it, sorry... it's not something I can change. — user541686, Apr 05 '12 at 08:58
@juanchopanza: It's prewritten code I can't change. (Namely: ATL/WTL/MFC) — user541686, Apr 05 '12 at 08:59
What I mean is, are there cases when returning a negative value makes sense, i.e. to communicate an error? — juanchopanza, Apr 05 '12 at 09:00
@juanchopanza: No, but that's how the code is already written in the framework, so I can't go ahead and change the signature just because I feel like it. :( — user541686, Apr 05 '12 at 09:01
No, but if a negative result has a meaning then you have to check for that before casting away :) — juanchopanza, Apr 05 '12 at 09:02
@juanchopanza Looking at [the docs](http://msdn.microsoft.com/en-us/library/aa300471(v=vs.60).aspx), it seems it will always be non-negative. — Peter Wood, Apr 05 '12 at 09:08
With the edit that is a very different question. I would do whatever fit into your error handling strategy. Write a function which asserts the value isn't negative and either use exceptions or return a boolean to allow handling the error. — Peter Wood, Apr 05 '12 at 09:57

GManNickG · Accepted Answer · 2012-04-05T09:17:00.313

6

Can you change GetLength()? Fundamentally, the issue is that length is never negative, and an unsigned type reflects that the best. Length shouldn't be measured with an int.

But other than that, all three of your solutions are identical. std::string::size_type is always std::size_t, and while I would use a static_cast, the C-style cast performs the same cast, in this case. Because you know that the returned length is never negative (ensure this, by the way; you never know what weird things people might do), you're completely safe simply casting the type:

return s2.size() > static_cast<std::size_t>(s1.GetLength());

If CString::GetLength can be negative, for some reason, then it's up to you to decide how to make that conversion from negative to positive. Truncate? Magnitude (absolute value)? Whatever you need.

If you're worried about bugs, either do an explicit check and throw an exception (depending on your domain, this may be too costly), or use assert. Generally, though, you should trust the documentation.

edited Apr 05 '12 at 09:17

answered Apr 05 '12 at 08:58

GManNickG

494,350
52
494
543

1

So you mean I should completely ignore the benefits of **`/RTCc`** here (in debug mode)? It seems quite helpful... (And no, I can't change that -- it's in a prewritten library, e.g. ATL/WTL/MFC.) – user541686 Apr 05 '12 at 08:59
2

I assume `CString` is from Windows MFC. – Peter Wood Apr 05 '12 at 09:00
1

@Mehrdad: If the check is important, I would add it myself and not rely on a specific compiler feature. If `GetLength()` is never negative, then just cast as you please. There won't be any data loss. – GManNickG Apr 05 '12 at 09:02
@GManNickG: It's only "important" for helping me find bugs, not "important" because it's something that should happen if the code was already correct. – user541686 Apr 05 '12 at 09:03
1

@Mehrdad: I don't really understand. If `GetLength()` is never negative, then casting to an unsigned type of equal (or greater) bit size never results in data loss. `std::size_t` is definitely as large as `int`, so you're totally safe doing the cast, no checks needed anywhere. – GManNickG Apr 05 '12 at 09:05
If `GetLength()` can return a negative number and it does return a negative number then it will be smaller than any `size_t`. Something like: `{ int l = s1.GetLength(); return l < 0 || static_cast(l) < s2.size(); }`. – CB Bailey Apr 05 '12 at 09:05
@GManNickG: See, it's not always that simple. If `GetLength()` has a bug and suddenly becomes negative (not likely in this particular example, but can easily happen in other cases -- especially if I'm overriding another method), then the check would obviously help me find it. But otherwise, it's not "supposed" to happen, so is it worth me (and the next reader) of going through the trouble of making my own cast every time? – user541686 Apr 05 '12 at 09:07
Pedantry: `std::size_t` is not _guaranteed_ to be larger than `int`. – CB Bailey Apr 05 '12 at 09:11
@Mehrdad: "If `GetLength()` has a bug..." then you'll find out when this code stops working. :) If you're really that fearful, add a check and throw an exception if it's negative. I would trust the documentation, though, especially for something so old and tested. – GManNickG Apr 05 '12 at 09:13
@CharlesBailey: Yes yes. One can `static_assert` that `std::size_t` bits is >= to `unsigned` bits if desired. – GManNickG Apr 05 '12 at 09:15

score 4 · Answer 2 · answered Apr 05 '12 at 09:18

4

Put the cast in its own function, with a comment:

std::string::size_type size(const CString& mfcString)
{
    // CString::GetLength is always non-negative
    // http://msdn.microsoft.com/en-us/library/aa300471(v=vs.60).aspx

    return static_cast<std::string::size_type>(mfcString.GetLength());
}

Then your code will be:

bool smaller(const CString& s1, const std::string& s2)
{
    return size(s1) < s2.size();
}

answered Apr 05 '12 at 09:18

Peter Wood

23,859
5
60
99

3

This is, in my opinion, the best answer so far. When dealing with a crufty API, do not litter your code with hacks. Instead, provide wrappers with a clean interface that encapsulate and hide the ugliness. Pure application of the **DRY** principle. – Matthieu M. Apr 05 '12 at 09:44

score 2 · Answer 3 · answered Apr 05 '12 at 09:06

2

I'd say the proper solution is to change the signature of CString::getLength() to return an unsigned type. Given that you are takung you arguments by value you might want to consider providing a conversion constructor for CString taking a std::string (orherwise you should probably change the signature of you function to take its arguments by const&).

Personally I'd consider the operation a conversion rather than a cast and write it as such:

return std::string::size_type(s1.getLength())
    < s2.size();

answered Apr 05 '12 at 09:06

Dietmar Kühl

150,225
13
225
380

1

A cast *is* a conversion. You've merely change cast notation, they're [quite literally the same](http://stackoverflow.com/questions/1652396/what-is-the-difference-between-typevalue-and-typevalue). – GManNickG Apr 05 '12 at 09:09
I'm well aware that `T(x)` and `static_cast(x)` are equivalent to the compiler. However, deliberate notation can convey information to the human reader: there are other places, e.g. a downcast (after a corresponding implicit conversion) where I'm using `static_cast(x)` to draw the attention to the fact that this isn't just patching up an otherwise implicit conversion. – Dietmar Kühl Apr 05 '12 at 09:58

score 0 · Answer 4 · answered Apr 05 '12 at 11:33

0

change method CString::getLength() and return an unsigned type.

try this

return std::string::size_type(s1.getLength()) < s2.size();

answered Apr 05 '12 at 11:33

CodingRat

1,934
3
23
43

How to cast properly? (Or should I even do it?)

Edit:

4 Answers4