Design of Matrix class

[kkarbowiak]

Looks like the design of Matrix class could be improved. Here are some points:

1. Matrix is a template class, but is only instantiated for uint32_t (in fact it's UserCost, UserDuration and UserDistance, but all boil down to the same type).
2. The implementation is partly in source file, which requires explicit template instantiation (see:

   vroom/src/structures/generic/matrix.cpp

   Line 36 in 176f57a

   template class Matrix<UserCost>;

   ) to prevent linking errors. The instantation is for UserCost, which automatically works for UserDuration and UserDistance due to all three being an alias for uint32_t. Adding explicit instantiation for the latter two types for clarity results in compiler error (duplicate explicit instantiation of ‘class vroom::Matrix<unsigned int>’).
3. Trying to reuse the class for any other type is not possible due to linker errors.
4. The constructors could be cleaned up a bit.

I propose to leave Matrix as a template class but move the implementation entirely to the header file.

[kkarbowiak]

Looks like there are some other template functions implemented in source files together with explicit template instantiation, e.g.

vroom/src/structures/vroom/tw_route.cpp

Lines 1441 to 1504 in 176f57a

	template bool
	TWRoute::is_valid_addition_for_tw(const Input& input,
	const Amount& delivery,
	const std::vector<Index>::iterator first_job,
	const std::vector<Index>::iterator last_job,
	const Index first_rank,
	const Index last_rank,
	bool check_max_load) const;
	template bool TWRoute::is_valid_addition_for_tw(
	const Input& input,
	const Amount& delivery,
	const std::vector<Index>::reverse_iterator first_job,
	const std::vector<Index>::reverse_iterator last_job,
	const Index first_rank,
	const Index last_rank,
	bool check_max_load) const;
	template bool TWRoute::is_valid_addition_for_tw(
	const Input& input,
	const Amount& delivery,
	const std::array<Index, 1>::const_iterator first_job,
	const std::array<Index, 1>::const_iterator last_job,
	const Index first_rank,
	const Index last_rank,
	bool check_max_load) const;
	template bool TWRoute::is_valid_addition_for_tw(
	const Input& input,
	const Amount& delivery,
	const std::vector<Index>::const_iterator first_job,
	const std::vector<Index>::const_iterator last_job,
	const Index first_rank,
	const Index last_rank,
	bool check_max_load) const;
	template void TWRoute::replace(const Input& input,
	const Amount& delivery,
	const std::vector<Index>::iterator first_job,
	const std::vector<Index>::iterator last_job,
	const Index first_rank,
	const Index last_rank);
	template void
	TWRoute::replace(const Input& input,
	const Amount& delivery,
	const std::vector<Index>::const_iterator first_job,
	const std::vector<Index>::const_iterator last_job,
	const Index first_rank,
	const Index last_rank);
	template void
	TWRoute::replace(const Input& input,
	const Amount& delivery,
	const std::vector<Index>::reverse_iterator first_job,
	const std::vector<Index>::reverse_iterator last_job,
	const Index first_rank,
	const Index last_rank);
	template void
	TWRoute::replace(const Input& input,
	const Amount& delivery,
	const std::array<Index, 1>::const_iterator first_job,
	const std::array<Index, 1>::const_iterator last_job,
	const Index first_rank,
	const Index last_rank);

What is the intention of this?

[jcoupey]

I propose to leave Matrix as a template class but move the implementation entirely to the header file.

Makes sense. As I understand it, this will allow the direct template instantiation in compilation units using the Matrix class, along with potential compiler optimizations.

[jcoupey]

The other explicit template instantiation occurrences (TWRoute::is_valid_addition_for_tw and the likes) are meant to allow using those functions in a wide range of contexts with const/non-const foward/reverse iterators. Happy to discuss a better way to do it. Obviously #1090 could make this simpler too.

[kkarbowiak]

I propose to leave Matrix as a template class but move the implementation entirely to the header file.

Makes sense. As I understand it, this will allow the direct template instantiation in compilation units using the Matrix class, along with potential compiler optimizations.

Exactly this.

[kkarbowiak]

The other explicit template instantiation occurrences (TWRoute::is_valid_addition_for_tw and the likes) are meant to allow using those functions in a wide range of contexts with const/non-const foward/reverse iterators.

This I understand.

Happy to discuss a better way to do it. Obviously #1090 could make this simpler too.

A common practice is to implement template functions in header files. This allows the compiler to instantiate the template as needed in translation units and avoids the need for explicit template instantiations. Potential drawbacks include increased compilation time and increased binary size.

I did not make myself clear but I meant to ask whether there was any reason to have the implementation in source file (with explicit template instantiation) as opposed to have the implementation in header file.

[jcoupey]

whether there was any reason to have the implementation in source file (with explicit template instantiation) as opposed to have the implementation in header file

I kind of made it this way to keep some level of separation between definition in headers and actual implementation. I have no strong feeling about this though and there are other situations where we moved things to the headers for optimization purposes. Yet, the implementation for TWRoute::replace and TWRoute::is_valid_addition_for_tw being much bigger, I guess I'd be a bit more hesitant to move them to the header than with the Matrix class. I realize this is rather subjective.

[jcoupey]

Done in #1091.