MSVC warnings

[davidebeatrici]

From https://github.com/tnagler/quickpool/runs/5404950620:

test.cpp(90,42): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [build\quickpool_test.vcxproj]
test.cpp(104,47): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [build\quickpool_test.vcxproj]
test.cpp(125,1): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [build\quickpool_test.vcxproj]
test.cpp(126,14): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [build\quickpool_test.vcxproj]
test.cpp(141,1): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [build\quickpool_test.vcxproj]
test.cpp(142,14): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [build\quickpool_test.vcxproj]
quickpool.hpp(919,31): warning C4244: 'argument': conversion from '__int64' to 'int', possible loss of data [build\quickpool_test.vcxproj]
test.cpp(174): message : see reference to function template instantiation 'void quickpool::ThreadPool::parallel_for_each<std::vector<size_t,std::allocator<size_t>>,main::<lambda_37fca2606d1d166ca50f88b50c00b954>>(Items &,UnaryFunction)' being compiled [build\quickpool_test.vcxproj]
          with
          [
              Items=std::vector<size_t,std::allocator<size_t>>,
              UnaryFunction=main::<lambda_37fca2606d1d166ca50f88b50c00b954>
          ]
quickpool.hpp(182,1): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [build\quickpool_test.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.31.31103\include\xmemory(572): message : see reference to function template instantiation 'void quickpool::mem::aligned::allocator<quickpool::loop::Worker<UnaryFunction>,64>::construct<_Ty,unsigned __int64,unsigned __int64,const Function&>(U *,unsigned __int64 &&,unsigned __int64 &&,const Function &)' being compiled [build\quickpool_test.vcxproj]
          with
          [
              UnaryFunction=main::<lambda_911885787203d9d13cac60ed82b30719>,
              _Ty=quickpool::loop::Worker<main::<lambda_911885787203d9d13cac60ed82b30719>>,
              Function=main::<lambda_911885787203d9d13cac60ed82b30719>,
              U=quickpool::loop::Worker<main::<lambda_911885787203d9d13cac60ed82b30719>>
          ]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.31.31103\include\xmemory(572): message : see reference to function template instantiation 'void quickpool::mem::aligned::allocator<quickpool::loop::Worker<UnaryFunction>,64>::construct<_Ty,unsigned __int64,unsigned __int64,const Function&>(U *,unsigned __int64 &&,unsigned __int64 &&,const Function &)' being compiled [build\quickpool_test.vcxproj]
          with
          [
              UnaryFunction=main::<lambda_911885787203d9d13cac60ed82b30719>,
              _Ty=quickpool::loop::Worker<main::<lambda_911885787203d9d13cac60ed82b30719>>,
              Function=main::<lambda_911885787203d9d13cac60ed82b30719>,
              U=quickpool::loop::Worker<main::<lambda_911885787203d9d13cac60ed82b30719>>
          ]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.31.31103\include\vector(607): message : see reference to function template instantiation 'void std::_Normal_allocator_traits<_Alloc>::construct<_Ty,unsigned __int64,unsigned __int64,const Function&>(_Alloc &,_Ty *,unsigned __int64 &&,unsigned __int64 &&,const Function &)' being compiled [build\quickpool_test.vcxproj]
          with
          [
              _Alloc=quickpool::mem::aligned::allocator<quickpool::loop::Worker<main::<lambda_911885787203d9d13cac60ed82b30719>>,64>,
              _Ty=quickpool::loop::Worker<main::<lambda_911885787203d9d13cac60ed82b30719>>,
              Function=main::<lambda_911885787203d9d13cac60ed82b30719>
          ]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.31.31103\include\vector(607): message : see reference to function template instantiation 'void std::_Normal_allocator_traits<_Alloc>::construct<_Ty,unsigned __int64,unsigned __int64,const Function&>(_Alloc &,_Ty *,unsigned __int64 &&,unsigned __int64 &&,const Function &)' being compiled [build\quickpool_test.vcxproj]
          with
          [
              _Alloc=quickpool::mem::aligned::allocator<quickpool::loop::Worker<main::<lambda_911885787203d9d13cac60ed82b30719>>,64>,
              _Ty=quickpool::loop::Worker<main::<lambda_911885787203d9d13cac60ed82b30719>>,
              Function=main::<lambda_911885787203d9d13cac60ed82b30719>
          ]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.31.31103\include\vector(625): message : see reference to function template instantiation 'void std::vector<quickpool::loop::Worker<UnaryFunction>,quickpool::mem::aligned::allocator<quickpool::loop::Worker<UnaryFunction>,64>>::_Emplace_back_with_unused_capacity<_Ty,_Ty,const Function&>(_Ty &&,_Ty &&,const Function &)' being compiled [build\quickpool_test.vcxproj]
          with
          [
              UnaryFunction=main::<lambda_911885787203d9d13cac60ed82b30719>,
              _Ty=size_t,
              Function=main::<lambda_911885787203d9d13cac60ed82b30719>
          ]
quickpool.hpp(384): message : see reference to function template instantiation 'void std::vector<quickpool::loop::Worker<UnaryFunction>,quickpool::mem::aligned::allocator<quickpool::loop::Worker<UnaryFunction>,64>>::emplace_back<size_t,size_t,const Function&>(size_t &&,size_t &&,const Function &)' being compiled [build\quickpool_test.vcxproj]
          with
          [
              UnaryFunction=main::<lambda_911885787203d9d13cac60ed82b30719>,
              Function=main::<lambda_911885787203d9d13cac60ed82b30719>
          ]
quickpool.hpp(898): message : see reference to function template instantiation 'std::shared_ptr<std::vector<quickpool::loop::Worker<UnaryFunction>,quickpool::mem::aligned::allocator<quickpool::loop::Worker<UnaryFunction>,64>>> quickpool::loop::create_workers<UnaryFunction>(const Function &,int,int,size_t)' being compiled [build\quickpool_test.vcxproj]
          with
          [
              UnaryFunction=main::<lambda_911885787203d9d13cac60ed82b30719>,
              Function=main::<lambda_911885787203d9d13cac60ed82b30719>
          ]
test.cpp(104): message : see reference to function template instantiation 'void quickpool::ThreadPool::parallel_for<main::<lambda_911885787203d9d13cac60ed82b30719>>(int,int,UnaryFunction)' being compiled [build\quickpool_test.vcxproj]
          with
          [
              UnaryFunction=main::<lambda_911885787203d9d13cac60ed82b30719>
          ]

[davidebeatrici]

@tnagler Should we use size_t instead of int (or other 4 bytes types)?

[tnagler]

As far as I can see, arguments of parallel_for() etc could be long, but then there are other problems:

• narrowing from unsigned long to signed long can cause negative signs after conversion (probably gives another warning).
• the parallel loop algorithm is no longer lock-free.

Best way I see to "fix" this is to static_cast<int>(x.size()) everywhere in test.cpp, but what's the point then.

[davidebeatrici]

Isn't std::atomic< std::size_t > supposed to be lock-free on 64 bit machines?

According to std::atomic< T >::is_lock_free() and std::atomic< T >::is_always_lock_free, it is on mine:

#include <atomic>
#include <iostream>

int main() {
	std::atomic_size_t size;

	std::cout << "is_lock_free(): " << size.is_lock_free() << std::endl;
	std::cout << "is_always_lock_free: " << size.is_always_lock_free << std::endl;

	return 0;
}

is_lock_free(): 1
is_always_lock_free: 1

[tnagler]

Yes but the loop algorithm relies on:

quickpool/quickpool.hpp

Lines 260 to 264 in 0a6b243

	struct State
	{
	int pos; //!< position in the loop range
	int end; //!< end of range assigned to worker
	};

This struct as a whole still allows for lock-free atomic operations. This is checked at the beginning of the unit tests:

quickpool/test.cpp

Lines 9 to 11 in 0a6b243

	mem::aligned::atomic<loop::State> test{};
	std::cout << "* [quickpool] lock free: "
	<< (test.is_lock_free() ? "yes\n" : "no\n");

If you replace int by a larger type that's no longer the case.

[davidebeatrici]

Oh, I see.

In that case I propose to use uint32_t instead, what do you think?

[tnagler]

Well that changes the functionality of the library, because loop ranges can no longer run over negative indexes. That isn't an issue in 99% of the cases, but it is a limitation. Unfortunately there's a trade-off here that we cannot avoid easily.

I personally don't really mind the warning about a possible narrowing conversion, because it does what it should - make a user aware of a potential, but unlikely bug in their code. It's the user's responsibility to turn such warnings on when there's a possibility that the loop range runs over billions of indexes. Making that bug impossible would be better of course.

One could keep the functionality and make it "safe" by 1) accepting long long as function arguments, 2) checking whether the numbers lie in int range, 3) throw an error if not, narrow explicitly to int if they do. Computational overhead is probably negligible, but it isn't pretty. "Safety" only applies to runtime and does only prevent unexpected results (instead of giving intended results). Another downside is that the function signature is misleading and this whole process needs to be explained (and found) in the docs.

An extension of the above that resolves the downsides would be to 1) allow long long arguments, 2) if arguments go beyond int range, convert to multiple int ranges, each stored a long with a long long offset, 3) run an outer loop over offsets + corresponding range, inner loop is current parallel_for called on one of the int-int ranges. This is something I could live with because it both adds to the current functionality and makes it more safe.

[davidebeatrici]

Sounds good, let's stick to fixed integer types though (int32_t and int64_t).