Acceptable fix for most signed / unsigned alerts?

Question

I myself am convinced that in a project in which I work on whole signs, the best choice is in most cases, although the value contained inside can never be negative. (Simplification of reverse for loops, less chance of errors, etc., In particular, for integers that can only contain values ​​between 0 and, say, 20).

Most of the places where this does not happen is a simple iteration of std :: vector, often it used to be an array and was later changed to std :: vector. So these loops usually look like this:

for (int i = 0; i < someVector.size(); ++i) { /* do stuff */ } 

Because this pattern is used so often, the amount of compiler warning spam about this comparison between a signed and an unsigned type tends to obscure more useful warnings. Note that we definitely do not have vectors with more than INT_MAX elements and note that so far we have used two ways to fix the compiler warning:

 for (unsigned i = 0; i < someVector.size(); ++i) { /*do stuff*/ } 

This usually works, but it can be interrupted if the loop contains any code like if (i-1> = 0) ... etc.

 for (int i = 0; i < static_cast<int>(someVector.size()); ++i) { /*do stuff*/ } 

This change has no side effects, but makes the cycle much less readable. (And it prints more.)

So, I came up with the following idea:

 template <typename T> struct vector : public std::vector<T> { typedef std::vector<T> base; int size() const { return base::size(); } int max_size() const { return base::max_size(); } int capacity() const { return base::capacity(); } vector() : base() {} vector(int n) : base(n) {} vector(int n, const T& t) : base(n, t) {} vector(const base& other) : base(other) {} }; template <typename Key, typename Data> struct map : public std::map<Key, Data> { typedef std::map<Key, Data> base; typedef typename base::key_compare key_compare; int size() const { return base::size(); } int max_size() const { return base::max_size(); } int erase(const Key& k) { return base::erase(k); } int count(const Key& k) { return base::count(k); } map() : base() {} map(const key_compare& comp) : base(comp) {} template <class InputIterator> map(InputIterator f, InputIterator l) : base(f, l) {} template <class InputIterator> map(InputIterator f, InputIterator l, const key_compare& comp) : base(f, l, comp) {} map(const base& other) : base(other) {} }; // TODO: similar code for other container types 

What you see is basically STL classes with methods that return the size_type parameter overridden to return only "int". Constructors are necessary because they are not inherited.

What would you think of it as a developer if you saw such a solution in the existing code base?

Could you think: “Yes, they redefine STL, what a huge WTF!”, Or do you think that this is a simple solution to prevent errors and increase readability. Or maybe you would prefer that we spend (half) a day or so changing all these loops to use std :: vector <> :: iterator?

(In particular, if this solution was combined with the prohibition of using unsigned types for anything other than raw data (for example, unsigned char) and bit masks.)

Answer 1

I made this community wiki ... Please edit it. I no longer agree with the advice against "int". Now I think it's not bad.

Yes, I agree with Richard. You should never use 'int' as a count variable in such a loop. The following is an example of how you can perform various loops using indexes (although there are few reasons, this can sometimes be useful).

Forward

 for(std::vector<int>::size_type i = 0; i < someVector.size(); i++) { /* ... */ } 

Back

You can do this with perfectly defined behaivor:

 for(std::vector<int>::size_type i = someVector.size() - 1; i != (std::vector<int>::size_type) -1; i--) { /* ... */ } 

Soon, when C ++ 1x (the next version of C ++) goes well, you can do it like this:

 for(auto i = someVector.size() - 1; i != (decltype(i)) -1; i--) { /* ... */ } 

Decreasing below 0 will cause me to turn around because it is unsigned.

But unsigned will make slavep errors in

This should never be an argument to make it incorrect (using 'int' ).

Why not use std :: size_t above?

The C ++ standard defines in 23.1 p5 Container Requirements that T::size_type for T is some Container , that this type represents a certain specific integrity type of an unsigned implementation. Now, using std::size_t for i above, you can disable errors. If T::size_type smaller or larger than std::size_t , then it overflows i or does not even get up to (std::size_t)-1 if someVector.size() == 0 . Similarly, the loop condition would be completely violated.

Answer 2

Do not publish publicly from STL containers. They have non-virtual destructors that cause undefined behavior if someone deletes one of your objects using a pointer to the base. If you must get, for example, from a vector, do it privately and expose the parts that you need to expose with using declarations.

Here, I would simply use size_t as a loop variable. It is simple and straightforward. A poster that commented that using the int index provides you as n00b is correct. However, using an iterator to loop over a vector provides you with a slightly more experienced n00b - someone who doesn't understand that the index operator for a vector is constant time. ( vector<T>::size_type is an accurate, but not necessarily detailed IBO).

Answer 3

Although I don’t think that “using iterators, otherwise you are looking at n00b” is a good solution, the output from std :: vector looks much worse.

First, developers expect the vector to be std: .vector, and map to std :: map. Secondly, your solution does not scale for other containers or for other classes / libraries that interact with containers.

Yes, iterators are ugly, iterator loops are not very readable, and typedefs only cover up the mess. But at least they are scalable, and they are a canonical solution.

My decision? macro stl for everyone. This is not without problems (mostly a macro, yuck), but he understands the meaning. It is not as advanced as, for example, this one , but it does the job.

Answer 4

Definitely use an iterator. Soon you will be able to use the “auto” type for better readability (one of your problems):

 for (auto i = someVector.begin(); i != someVector.end(); ++i) 

Answer 5

Skip index

The easiest way is to work around the problem using iterators, ranges for loops, or algorithms:

 for (auto it = begin(v); it != end(v); ++it) { ... } for (const auto &x : v) { ... } std::for_each(v.begin(), v.end(), ...); 

This is a good solution if you really do not need the index value. It also handles reverse loops easily.

Use a suitable unsigned type

Another approach is to use a container size type.

 for (std::vector<T>::size_type i = 0; i < v.size(); ++i) { ... } 

You can also use std::size_t (from <cstddef>). There are those who (correctly) point out that std::size_t may not be the same type as std::vector<T>::size_type (although this is usually the case). However, you can be sure that the size_type container size_type fit in std::size_t . So, everything is in order if you do not use certain styles for reverse loops. My preferred back loop style:

 for (std::size_t i = v.size(); i-- > 0; ) { ... } 

In this style, you can safely use std::size_t even if it is a larger type than std::vector<T>::size_type . The inverse loop style shown in some other answers requires casting -1 to exactly the right type and therefore cannot use a simpler type for std::size_t .

Use a signed type (carefully!)

If you really want to use a signed type (or if your leadership style practically requires one ), for example int , then you can use this tiny function template that checks the basic assumption in debug lines and makes the conversion explicit so that you don't get a compiler message:

 #include <cassert> #include <cstddef> #include <limits> 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); } 

Now you can write:

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

Or reverse loops in the traditional way:

 for (int i = size_as_int(v) - 1; i >= 0; --i) { ... } 

The size_as_int trick only size_as_int up a bit more than loops with implicit conversions, you check the basic assumption at runtime, you turn off the compiler warning by explicit actuation, you get the same speed as non-debug builds because it will almost certainly be built in, and the optimized object code should not be anymore because the template does nothing that the compiler no longer does implicitly.

Answer 6

vector.size() returns a size_t var, so just change int to size_t and everything should be fine.

Richard's answer is more correct, except that he works hard for a simple loop.

Answer 7

You have mixed up the problem.

Using the variable size_t is preferable, but if you do not trust your programmers to use the unsigned correctly, go with the cast and just handle the ugliness. Get an intern to change them all and not worry about it after that. Include warnings as errors, and no new ones will be squeezed. Now your loops may be ugly, but you can understand this as the consequences of your religious position with respect to the signed and the unsigned.