How do I deal with "signed/unsigned mismatch" warnings (C4018)?

Question

I work with a lot of calculation code written in c++ with high-performance and low memory overhead in mind. It uses STL containers (mostly std::vector) a lot, and iterates over that containers almost in every single function.

The iterating code looks like this:

for (int i = 0; i < things.size(); ++i)
{
    // ...
}

But it produces the signed/unsigned mismatch warning (C4018 in Visual Studio).

Replacing int with some unsigned type is a problem because we frequently use OpenMP pragmas, and it requires the counter to be int.

I'm about to suppress the (hundreds of) warnings, but I'm afraid I've missed some elegant solution to the problem.

On iterators. I think iterators are great when applied in appropriate places. The code I'm working with will never change random-access containers into std::list or something (so iterating with int i is already container agnostic), and will always need the current index. And all the additional code you need to type (iterator itself and the index) just complicates matters and obfuscates the simplicity of the underlying code.

Solution 1

It's all in your things.size() type. It isn't int, but size_t (it exists in C++, not in C) which equals to some "usual" unsigned type, i.e. unsigned int for x86_32.

Operator "less" (<) cannot be applied to two operands of different sign. There's just no such opcodes, and standard doesn't specify, whether compiler can make implicit sign conversion. So it just treats signed number as unsigned and emits that warning.

It would be correct to write it like

for (size_t i = 0; i < things.size(); ++i) { /**/ }

or even faster

for (size_t i = 0, ilen = things.size(); i < ilen; ++i) { /**/ }

Solution 2

Ideally, I would use a construct like this instead:

for (std::vector<your_type>::const_iterator i = things.begin(); i != things.end(); ++i)
{
  // if you ever need the distance, you may call std::distance
  // it won't cause any overhead because the compiler will likely optimize the call
  size_t distance = std::distance(things.begin(), i);
}

This a has the neat advantage that your code suddenly becomes container agnostic.

And regarding your problem, if some library you use requires you to use int where an unsigned int would better fit, their API is messy. Anyway, if you are sure that those int are always positive, you may just do:

int int_distance = static_cast<int>(distance);

Which will specify clearly your intent to the compiler: it won't bug you with warnings anymore.

Solution 3

If you can't/won't use iterators and if you can't/won't use std::size_t for the loop index, make a .size() to int conversion function that documents the assumption and does the conversion explicitly to silence the compiler warning.

#include <cassert>
#include <cstddef>
#include <limits>

// When using int loop indexes, use size_as_int(container) instead of
// container.size() in order to document the inherent assumption that the size
// of the container can be represented by an int.
template <typename ContainerType>
/* constexpr */ int size_as_int(const ContainerType &c) {
    const auto size = c.size();  // if no auto, use `typename ContainerType::size_type`
    assert(size <= static_cast<std::size_t>(std::numeric_limits<int>::max()));
    return static_cast<int>(size);
}

Then you write your loops like this:

for (int i = 0; i < size_as_int(things); ++i) { ... }

The instantiation of this function template will almost certainly be inlined. In debug builds, the assumption will be checked. In release builds, it won't be and the code will be as fast as if you called size() directly. Neither version will produce a compiler warning, and it's only a slight modification to the idiomatic loop.

If you want to catch assumption failures in the release version as well, you can replace the assertion with an if statement that throws something like std::out_of_range("container size exceeds range of int").

Note that this solves both the signed/unsigned comparison as well as the potential sizeof(int) != sizeof(Container::size_type) problem. You can leave all your warnings enabled and use them to catch real bugs in other parts of your code.

Solution 4

You can use:

1. size_t type, to remove warning messages
2. iterators + distance (like are first hint)
3. only iterators
4. function object

For example:

// simple class who output his value
class ConsoleOutput
{
public:
  ConsoleOutput(int value):m_value(value) { }
  int Value() const { return m_value; }
private:
  int m_value;
};

// functional object
class Predicat
{
public:
  void operator()(ConsoleOutput const& item)
  {
    std::cout << item.Value() << std::endl;
  }
};

void main()
{
  // fill list
  std::vector<ConsoleOutput> list;
  list.push_back(ConsoleOutput(1));
  list.push_back(ConsoleOutput(8));

  // 1) using size_t
  for (size_t i = 0; i < list.size(); ++i)
  {
    std::cout << list.at(i).Value() << std::endl;
  }

  // 2) iterators + distance, for std::distance only non const iterators
  std::vector<ConsoleOutput>::iterator itDistance = list.begin(), endDistance = list.end();
  for ( ; itDistance != endDistance; ++itDistance)
  {
    // int or size_t
    int const position = static_cast<int>(std::distance(list.begin(), itDistance));
    std::cout << list.at(position).Value() << std::endl;
  }

  // 3) iterators
  std::vector<ConsoleOutput>::const_iterator it = list.begin(), end = list.end();
  for ( ; it != end; ++it)
  {
    std::cout << (*it).Value() << std::endl;
  }
  // 4) functional objects
  std::for_each(list.begin(), list.end(), Predicat());
}

Solution 5

C++20 has now std::cmp_less

In c++20, we have the standard constexpr functions

std::cmp_equal
std::cmp_not_equal
std::cmp_less
std::cmp_greater
std::cmp_less_equal
std::cmp_greater_equal

added in the <utility> header, exactly for this kind of scenarios.

Compare the values of two integers t and u. Unlike builtin comparison operators, negative signed integers always compare less than (and not equal to) unsigned integers: the comparison is safe against lossy integer conversion.

That means, if (due to some wired reasons) one must use the i as integer, the loops, and needs to compare with the unsigned integer, that can be done:

#include <utility> // std::cmp_less

for (int i = 0; std::cmp_less(i, things.size()); ++i)
{
    // ...
}

---

This also covers the case, if we mistakenly static_cast the -1 (i.e. int)to unsigned int. That means, the following will not give you an error:

static_assert(1u < -1);

But the usage of std::cmp_less will

static_assert(std::cmp_less(1u, -1)); // error

View more solutions

Comments

[Billy ONeal]

Can you post an example where the OpenMP pragma prevents you using an unsigned type? According to this it should work for any intergal type, not just int.

[bcsanches]

I believe this question is better for stackoverflow.

[Adrian McCarthy]

int and std::vector<T>::size_type may also be different in size as well as in signedness. For example, on a LLP64 system (like 64-bit Windows), sizeof(int) == 4 but sizeof(std::vector<T>::size_type) == 8.

[Adrian McCarthy]

possible duplicate of acceptable fix for majority of signed/unsigned warnings?

[CinCout]

possible duplicate of stackoverflow.com/questions/8188401/…

[KindDragon]

Check this suggestion visualstudio.uservoice.com/forums/121579-visual-studio-ide/…

[Andrew T]

I always need the distance. Maybe static_cast<int>(things.size()) could be the solutions, if there are no others.

[Billy ONeal]

@Andrew: If you do decide to suppress the warning, the best way would probably be to use a compiler specific pragma (on MSVC this will be a #pragma warning(push) #pragma warning(disable: 4018) /* ... function */ #pragma warning(pop)) rather than to use a needless cast. (Casts hide legitimate errors, m'kay? ;) )

[Billy ONeal]

Not true. Cast != conversion. The warning is warning about an implicit conversion allowed by the standard which may be unsafe. A cast, however, invites explicit conversions to the party which can be more unsafe than what was originally talked about by the warning. Strictly speaking, at least on x86, no conversion is going to happen at all -- the processor does not care whether you are treating a specific part of memory as signed or unsigned, so long as you use the correct instructions to work with it.

[Raedwald]

-1 no, it is not size_t. Its is std::vector< THING >::size_type.

[Adrian McCarthy]

@Raedwald: While you're technically correct, it's hard to imagine how a standards-compliant implementation could end up with different underlying types for std::size_t and std::vector<T>::size_type.

[Shoaib]

why is ++i being considered better? In for loops isn't there "no" difference?

[Kat]

@ShoaibHaider, it doesn't matter at all for primitives where the return value isn't being used. However, for custom types (where the operator is overloaded), post increment is almost always less efficient (since it has to make a copy of the object before it was incremented). Compilers can't (necessarily) optimize for custom types. So the only advantage is consistency (of primitives vs custom types).

[emlai]

@AdrianMcCarthy std::vector<T>::size_type depends on the allocator used. If the default allocator (std::allocator<T>) is used, then size_type == std::size_t. For custom allocators, size_type might be something else than std::size_t.

[Adrian McCarthy]

@zenith: Yes, you're right. My statement holds only for the default allocator. A custom allocator might use something other than std::size_t, but I believe it would still have to be an unsigned integral type, and it probably couldn't represent a larger range than std::size_t, so it's still safe to use std::size_t as the type for a loop index.

[parker.sikand]

+1 for C++11. Unless there is a good reason you can't use C++11, I think it's best to use the new features... they are meant to be a great help. And definitely use for (auto thing : vector_of_things) if you don't actually need the index.

[Adrian McCarthy]

But that solves only the signedness problem. It doesn't help if size() returns a type larger than unsigned int, which is extremely common.

[Adrian McCarthy]

I'm unclear what is meant by this part of the answer: "standard doesn't specify[...] whether compiler can make implicit sign conversion." The standard requires the conversion ("[I]f the operand that has unsigned integer type has rank greater than or equal to the rank of the type of the other operand, the operand with signed integer type shall be converted to the type of the operand with unsigned integer type.")

[No-Bugs Hare]

When being container-agnostic causes O(N^2) complexity (and in your example it does, as distance() is O(N) for list<>), I am not so sure it is an advantage :-(.

[ereOn]

@No-BugsHare That's exactly the point: we can't be sure. If the OP has few elements, then it's probably great. If he has millions of those, probably not so much. Only profiling can tell in the end but the good news is: you can always optimize maintainable code!

[tjwrona1992]

@TREMOR, ++i is generally considered better because i++ is typically implemented in terms of ++i. This means that by using ++i you avoid one additional function call and get slightly better performance (although it is negligible in most cases)