C/C++: Which conversion warnings make sense in practice?

Question

C/C++ implicit conversions can cause bugs. Here's one example:

int foo, bar;
scanf("%d", &foo);
scanf("%d", &bar);
float foobar = foo / bar;

If I input 7 and 2, it's not 3.5 as expected - it's 3 -> bug (let's ignore the buffer overflow).

gcc's -Wconversion warns about this kind of stuff, so I turned it on. But it just seems to me like it's going over the top with the warnings. For instance, this:

int foo;
float bar;
scanf("%d", &foo);
scanf("%f", &bar);
float foobar = foo / bar;

Causes a warning:

warning: conversion to 'float' from 'int' may alter its value [-Wconversion]

Even though it works as intended, returning 3.5 when I input 7 and 2. I know that float cannot represent all possible int values precisely, but I'm questioning if adding a cast here does anything to help - other than adding more code. In the example above, casting one of the int values in the expression to float actually changed the result, replacing an implicit cast by an explicit one in this example doesn't.

So much for float to int conversion warnings, but that's not all. There's also sign conversion warnings. Traditional example:

std::vector<char> chars = get_chars();
for (int i = 0; i < chars.size(); i++)
 std::cout << chars[i] << std::endl;

Causes this warning:

warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

I could use the more cumbersome unsigned int for the index variable instead - which will require further casting when I do calculations with it. All for the unusual situation that I have between 2^31 + 1 and 2^32 elements in my vector and don't do any calculations with the index variable that cast it to int.

So, my question is: Which conversion warnings make sense in practice? Does adding explicit casts to fix them really improve anything?

• Charles Salvia: In your above vector iteration example you should use std::size_t, or better yet... iterators
• user4828: It's impossible to reduce this to a rule, it depends on the circumstances. For example, I work in bioinformatics and warnings about signed vs. unsigned comparisons have to be taken very seriously, because strings larger than 2^32 are common, and your loop would cause a runtime error when i overflowed. time, Casts are useful as a form of documentation to let future readers know that you are aware of the conversion, and are doing it deliberately, and not just being feckless.
• user53141: In C++, if you divide the int 7 by the int 2, getting 3 is neither a bug nor a buffer overflow. If you don't want integer math, you should convert everything to float.

Answer 1

It goes kind of like this.

1. The ability to use unsigned values and variables and real numbers of different precisions are not particularly common features in programming languages, but are critical in order for C/C++ to do its job.
2. Mixing arithmetic types like these is a major cause of bugs in real, commercial C/C++ software. Trust me, I've seen plenty.
3. So the C/C++ standards nail down conversion behaviour with great care. There are pages and pages on this stuff.
4. Therefore we ask the compiler to do as much as it possibly can to help us find and avoid bugs in the conversion world. Some things are errors (contravene the standard); others are warnings (default is associated with possible or likely bugs).
5. Now it is up to us to identify programming strategies that minimise the risk of bugs, and in particular that do not trigger warnings. That's our job.

Casts are bad. Every cast potentially papers over a bug or a warning or a design issue. We can't live without them, but the fewer the better. Most are avoidable, with proper design.

Your two examples all look like wrong code to me. In the first two examples you need to tell the compiler whether to use int, float or double arithmetic, as the defaults could easily be incorrect. The third example must use an unsigned loop variable. C/C++ is a very sharp knife and you need more care to avoid cutting yourself.

• mattnz: +1: Although I think of C++ as a chainsaw rather than a sharp knife - Its far from a surgeons scalpel, its crude, useful and powerful, can cut down a forest in a day or create great art in the hands of master - but will quickly take off an arm and a leg in the hands of the untrained or careless.
• david.pfx: @mattnz: Actually, it's both. It's like a machine shop full of tools with no safety guards. You will get hurt.
• Darius Duesentrieb: For everyone like me, wondering what art you can create with a chainsaw: en.wikipedia.org/wiki/Chainsaw_carving

Answer 2

Here's what decades of experience have taught me.

1. Turn on all the conversion warnings and tell the compiler to treat all warnings as errors.

2. The conversions that must happen should be made explicit in the code. This will silence the warnings. Call a constructor or use the appropriate cast. (The appropriate cast is usually static_cast, or, for a limited set of pointer conversions, reinterpret_cast but beware that type punning leads to undefined behavior.) Avoid C-style casts.

3. The C++-style casts are big and ugly, and that's a feature. They should stand out. If they're cluttering the code, that's a good sign you're using the wrong types or you have an impedance mismatch with an API you're using. Fix the underlying problem.

4. Literals should be of the appropriate type.

   int x = 0;
   int * p = nullptr; // not 0, not NULL
   char ch = '0円'; // not 0
   wchar_t wch = L'0円';
   // In a template<typename T> Foo ...
   T t = T{}; // default constructed
   T t = T{0}; // zero initialized
   

   Did you know the type of an integral literal can depend on its value and whether it's expressed in decimal, binary, octal, or hexadecimal?

5. Almost Always Auto.

   I acknowledge that this is not necessarily the most popular stance, but I haven't heard a persuasive argument from the anti-AAA (AAAA?) folks that isn't mitigated by other items on this list.

6. Consider using universal initialization, especially if you're forced into a situation where you cannot enable the conversion warnings.

7. Choose your types to match those used by the APIs you're using, even if you think the API made the wrong choice.

8. When feasible prefer algorithms, range-based for loops, or iterators over indexing.

   When you must use an index, its type should match the API for the container, like std::vector<T>::size_type. That can be unwieldy, so for the standard containers, I go with std::size_t. While not pedantically correct, it's pragmatically correct.

9. By default, mark your single-parameter constructors and conversion operators explicit. It may be reasonable to strategically remove the explicit requirement in a small number of cases. But try living with it for a while before deciding.

10. Use const aggressively. It avoids simple errors and improves code readability. It's much harder to insert const into existing code that it is to remove it or to add a non-const overload.

11. Remember that char is distinct from both signed char and unsigned char, even though it functions identically to one or the other.

    The character classification and conversion functions in the standard library (like std::isalpha and std::tolower) were designed to rely on integral promotions. If you're working with strictly 7-bit ASCII and/or on a platform that uses an unsigned representation for char you probably won't notice. But if your char is signed, you will need to do quite a bit of casting to use these functions correctly.

---

Rants Follow

It's ridiculous that you have to turn conversion warnings on. They should be on by default.

If your style guide requires you to write code that's incompatible with conversion warnings, there's a bug in your style guide. I've spent too much of my life chasing hard-to-find bugs that would have never been checked in if the warnings had been enabled.

I dislike the phrase "false positive" when applied to warnings. A warning message does not say "You've got a bug here." It says "your code here matches a pattern commonly correlated with bugs." If you inspect the code highlighted by a warning and determine that there is no bug, that's not a false positive. The fact is the code does match the suspicious pattern. That's a true positive.

I disagree with those (including prominent folks on the C++ standards committee) who think using unsigned types for sizes was a bad choice or leftover baggage from history. I don't find their arguments persuasive.

Even if you do think it's a mistake that std::size(container) returns an unsigned type, that's the API we have to work with. Pretending otherwise (without explicit conversions) requires explicit disabling conversion warnings. And even if you know that all your container sizes will always fit in an int, disabling the conversion warnings will hide insidious bugs caused by signed-versus-unsigned mismatches elsewhere in the code.

Answer 3

With GCC you'll find alot of the important warnings are already grouped together under -Wall and -Wextra, its also useful to use -Werror to turn warnings into errors.

Answer 4

The -Wconversion flag frequently gives false positives, so it may be a good idea to turn these warnings off unless you really need the rigor.

In most cases, I find -Wsign-conversion to be more useful as unintentional signed-to-unsigned conversions can often cause serious bugs.

It would be nice if GCC provided a separate flag that warns only about non-subtype conversions though (e.g. int to char).

Answer 5

You get the int-to-float warning because int > 2^24 are usually converted with an error. My rule is that you should use double unless you can give a good reason to use float. So what is your reason to use float?

For variables storing sizes, or number of things stored in memory, use size_t. It’s always the correct type to hold a size. int is wrong, unsigned int is slightly less wrong, size_t is right. Imagine having an array of chars, storing the size as int, and then you buy a new computer with 64GB of RAM and you have an array with tens of billions of elements. With size_t, it just works.