const-ref for optional allocator arguments?

[rwols]

In this issue: https://github.com/KhronosGroup/Vulkan-Hpp/issues/237, it was requested to have an optional Allocator argument for various functions that allocate std::vectors.

This works, but the argument for the allocator is taken by reference, not by const-reference. For instance, the method for allocating descriptor sets is:

  // wrapper function for command vkAllocateDescriptorSets, see https://registry.khronos.org/vulkan/specs/latest/man/html/vkAllocateDescriptorSets.html
  template <typename DescriptorSetAllocator,
            typename Dispatch,
            typename std::enable_if<std::is_same<typename DescriptorSetAllocator::value_type, VULKAN_HPP_NAMESPACE::DescriptorSet>::value, int>::type>
  VULKAN_HPP_NODISCARD VULKAN_HPP_INLINE typename ResultValueType<std::vector<VULKAN_HPP_NAMESPACE::DescriptorSet, DescriptorSetAllocator>>::type
                       Device::allocateDescriptorSets( const VULKAN_HPP_NAMESPACE::DescriptorSetAllocateInfo & allocateInfo,
                                    DescriptorSetAllocator &                                descriptorSetAllocator,
                                    Dispatch const &                                        d ) const
  {
    VULKAN_HPP_ASSERT( d.getVkHeaderVersion() == VK_HEADER_VERSION );
#  if ( VULKAN_HPP_DISPATCH_LOADER_DYNAMIC == 1 )
    VULKAN_HPP_ASSERT( d.vkAllocateDescriptorSets && "Function <vkAllocateDescriptorSets> requires <VK_VERSION_1_0>" );
#  endif

    std::vector<VULKAN_HPP_NAMESPACE::DescriptorSet, DescriptorSetAllocator> descriptorSets( allocateInfo.descriptorSetCount, descriptorSetAllocator );
    VULKAN_HPP_NAMESPACE::Result                                             result = static_cast<VULKAN_HPP_NAMESPACE::Result>( d.vkAllocateDescriptorSets(
      m_device, reinterpret_cast<const VkDescriptorSetAllocateInfo *>( &allocateInfo ), reinterpret_cast<VkDescriptorSet *>( descriptorSets.data() ) ) );
    VULKAN_HPP_NAMESPACE::detail::resultCheck( result, VULKAN_HPP_NAMESPACE_STRING "::Device::allocateDescriptorSets" );

    return VULKAN_HPP_NAMESPACE::detail::createResultValueType( result, std::move( descriptorSets ) );
  }

You can see here that the template type DescriptorSetAllocator must be passed by mutable reference as DescriptorSetAllocator &descriptorSetAllocator.

Now, the various constructors for the various datastructures in the std lib take their allocator argument by const-ref: https://en.cppreference.com/w/cpp/container/vector/vector.html

Would it make sense to make all the allocator arguments const& instead of &? I've taken to writing my functions taking a const Alloc& allocator type, and when such a const Alloc& allocator type is passed to allocateDescriptorSets, the template type is deduced to be const Alloc, which makes the return type be std::vector<vk::DescriptorSet, const Alloc>, which is not allowed.

If you pass a mutable Alloc& type to allocateDescriptorSets, an additional const would not make the function fail to compile because the type of DescriptorSetAllocator would still be Alloc. Let me make a table summary...

input arg from user	function input arg type	template	deduced type	vector return type
Alloc&	Alloc&	Alloc	Alloc	std::vector<vk::DescriptorSet, Alloc> (1) ✅
const Alloc&	Alloc&	Alloc	const Alloc	std::vector<vk::DescriptorSet, const Alloc> (1) ❌
Alloc&	const Alloc&	Alloc	Alloc	std::vector<vk::DescriptorSet, Alloc> (2) ✅
const Alloc&	const Alloc&	Alloc	Alloc	std::vector<vk::DescriptorSet, Alloc> (2) ✅

So I'm suggesting to take all allocator args by const-ref to avoid the situation in the second row. Alternatively, you could use std::decay_t<Alloc> or std::remove_const_t<Alloc> to remove the constness when defining the vector type.

[asuessenbach]

Thanks for reporting this issue. I'll have a closer look at it asap, but that still might take a little. But at first sight, it sounds reasonable.

[asuessenbach]

@rwols Would you like to check if #2386 actually resolves this issue?

[rwols]

I'll check it out soon. Thank you for making the change!