Create templated function `MakeFilled`

[Leengit]

Description

As @N-Dekker suggests in #3007,

Instead of adding yet another static Filled member function, maybe we could also add a generic MakeFilled function template that would support fixed ITK arrays, as well as std::array, for example:

auto spacing = MakeFilled<SpacingType>(1.0);             // = { 1.0, 1.0 }, for a 2D spacing
auto rgbPixel = MakeFilled<itk::RGBPixel<uint8_t>>(255); // = { 255, 255, 255 };
auto stdArray = MakeFilled<std::array<int, 4>>(42);      // = { 42, 42, 42, 42 };

Code that achieves that is

#include <type_traits>

template <typename TContainer>
TContainer
MakeFilled(const typename std::remove_reference<decltype(*TContainer{}.begin())>::type & value)
{
  typename std::enable_if<!TContainer{}.empty(), TContainer>::type result{};
  std::fill(result.begin(), result.end(), value);
  return result;
}

[Leengit]

Actually, I would dispense with the empty() check because maybe not all containers have that. And the ones that do have that, but don't declare it as constexpr, will give cryptic compiler errors. That they error is correct, but the messages are not necessarily helpful.

#include <type_traits>

template <typename TContainer>
TContainer
MakeFilled(const typename std::remove_reference<decltype(*TContainer{}.begin())>::type & value)
{
  TContainer result{};
  std::fill(result.begin(), result.end(), value);
  return result;
}

[N-Dekker]

Thanks @Leengit but I think it is important that containers like std::vector and itk::Array are excluded, one way or the other. MakeFilled should only be available for container types whose default initial size > 0. What about the following?

template <typename TContainer>
TContainer
MakeFilled(const std::remove_reference_t<decltype(*TContainer{}.begin())> & value)
{
  TContainer result;
  constexpr auto n = result.size();
  static_assert(n > 0, "The container must be non-empty by default!");
  std::fill_n(result.begin(), n, value);
  return result;
}

[Leengit]

Like empty(), size() will not be constexpr for the containers with a variable size. We would properly get compiler errors but they will not be the nice "The container must be non-empty by default!".

[Leengit]

With my compiler, SFINAE does not silently fail if there is also a failing static_assert; the static_assert is in force despite the earlier substitution failure. So a static_assert(false, "Bad!"); in the code that is a specialized template for zero-sized containers will warn us about, for example, std::vector<float> but will also falsely warn us about std::array<float, 4> before it realizes that the latter has a substitution failure.

If we can keep the string on the same line as the failing empty() call, even after clang-format has its way with it, then the following code at least shows the string to the user in the error messages -- with my compiler. YMMV.

#include <type_traits>

template <typename TContainer>
typename std::enable_if<!TContainer{}.empty() || "Container with zero elements at construction!", TContainer>::type
MakeFilled(const typename std::remove_reference<decltype(*TContainer{}.begin())>::type & value)
{
  TContainer result{};
  std::fill(result.begin(), result.end(), value);
  return result;
}

#include <array>
#include <vector>

int
main()
{
  auto a = MakeFilled<std::array<int, 4>>(8);
  auto b = MakeFilled<std::vector<int>>(3);
}

[N-Dekker]

Very clever 👍, however, personally I find a static_assert much easier to read than SFINAE. (I find SFINAE a rather complicated mechanism in general, and I would rather just use it to solve some particular overloading problems.)

Some small remarks:

the static_assert is in force despite the earlier substitution failure

I don't really understand. If the substitution failure works properly, the corresponding static_assert failure should not occur. 🤷

The std::enable_if (if still necessary) and std::remove_reference can be replaced by the corresponding C++14 aliases, std::enable_if_t and std::remove_reference_t, to get rid of the typename ... ::type. Like we did with pull request #2580 commit 0f139ccfff4f6908c5d6776dfd722f505bea8e71 "STYLE: Use C++14 template aliases instead of nested types"

The {} doesn't seem useful to me, here:

TContainer result{};
std::fill(result.begin(), result.end(), value);

The {} will typically take care of zero-initializing (or "value-initializing") all container elements. But then, all container elements will be set to value directly afterwards. So the zero-initialization was for nothing, then!

[Leengit]

the static_assert is in force despite the earlier substitution failure

I don't really understand. If the substitution failure works properly, the corresponding static_assert failure should not occur. shrug

I agree. But g++ 10.3.0 on Ubuntu 20.04 doesn't.

Very clever +1, however, personally I find a static_assert much easier to read than SFINAE. (I find SFINAE a rather complicated mechanism in general, and I would rather just use it to solve some particular overloading problems.)

In the present case, with g++ 10.3.0, there is no advantage to SFINAE. So, yes, there is no advantage to be gained from the complications of SFINAE in this case.

The std::enable_if (if still necessary) and std::remove_reference can be replaced by the corresponding C++14 aliases, std::enable_if_t and std::remove_reference_t, to get rid of the typename ... ::type. Like we did with pull request #2580 commit 0f139cc "STYLE: Use C++14 template aliases instead of nested types"

I like it.

The {} doesn't seem useful to me, here:

TContainer result{};
std::fill(result.begin(), result.end(), value);

The {} will typically take care of zero-initializing (or "value-initializing") all container elements. But then, all container elements will be set to value directly afterwards. So the zero-initialization was for nothing, then!

I like it.

[Leengit]

So, yes @N-Dekker code in https://github.com/InsightSoftwareConsortium/ITK/issues/3230#issuecomment-1050196001 looks like the best implementation.

[Leengit]

G++ bug report submitted as: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104691

[dzenanz]

Well, clang and msvc also [fail to compile this](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:1,endLineNumber:6,positionColumn:1,positionLineNumber:6,selectionStartColumn:1,selectionStartLineNumber:6,startColumn:1,startLineNumber:6),source:'%23include+%3Ctype_traits%3E%0A%23include+%3Calgorithm%3E%0A%23include+%3Carray%3E%0A%23include+%3Ciostream%3E%0A%0A%0Atemplate+%3Ctypename+TContainer%3E%0Astd::enable_if_t%3C!!TContainer%7B%7D.empty(),+TContainer%3E%0AMakeFilled(const+typename+std::remove_reference%3Cdecltype(*TContainer%7B%7D.begin())%3E::type+%26+value)%0A%7B%0A++TContainer+result%7B%7D%3B%0A++std::fill(result.begin(),+result.end(),+value)%3B%0A++return+result%3B%0A%7D%0A%0Atemplate+%3Ctypename+TContainer%3E%0Astd::enable_if_t%3Cstd::is_class%3Ctypename+TContainer::allocator_type%3E::value,+TContainer%3E%0AMakeFilled(const+typename+std::remove_reference%3Cdecltype(*TContainer%7B%7D.begin())%3E::type+%26+value)%0A%7B%0A++static_assert(false,+%22Substitution+failure+so+this+should+never+be+checked!!%22)%3B%0A%7D%0A%0A%0Aint%0Amain()%0A%7B%0A++using+ArrayWithPositiveSize+%3D+std::array%3Cint,+4%3E%3B%0A++ArrayWithPositiveSize+a+%3D+MakeFilled%3CArrayWithPositiveSize%3E(8)%3B%0A++std::cout+%3C%3C+%22a%5B%22+%3C%3C+a.size()+-+1+%3C%3C+%22%5D+%3D+%22+%3C%3C+a%5Ba.size()+-+1%5D+%3C%3C+std::endl%3B%0A%7D'),l:'5',n:'0',o:'C%2B%2B+source+%231',t:'0')),k:47.18651813767496,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((h:compiler,i:(compiler:clang1301,filters:(b:'0',binary:'1',commentOnly:'0',demangle:'0',directives:'0',execute:'1',intel:'0',libraryCode:'0',trim:'1'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'',selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1,tree:'1'),l:'5',n:'0',o:'x86-64+clang+13.0.1+(C%2B%2B,+Editor+%231,+Compiler+%231)',t:'0')),k:19.480148528991716,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compiler:1,editor:1,fontScale:14,fontUsePx:'0',tree:'1',wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+clang+13.0.1+(Compiler+%231)',t:'0')),k:33.33333333333333,l:'4',n:'0',o:'',s:0,t:'0')),l:'2',n:'0',o:'',t:'0')),version:4), so it might not be a bug in GCC.

[Leengit]

We could support variable-sized arrays as well.

#include <array>
#include <iomanip>
#include <iostream>
#include <type_traits>
#include <vector>

template <typename TContainer>
TContainer
MakeFilled(const std::remove_reference_t<decltype(*TContainer{}.begin())> & value)
{
  TContainer result;
  static_assert(result.size() > 0, "MakeFilled must have object with non-zero size");
  std::fill(result.begin(), result.end(), value);
  return result;
}

template <typename TContainer>
TContainer
MakeFilled(unsigned int VSize, const std::remove_reference_t<decltype(*TContainer{}.begin())> & value)
{
  TContainer result(VSize);
  std::fill(result.begin(), result.end(), value);
  return result;
}

int
main()
{
  auto a = MakeFilled<std::array<double, 3>>(3.141592653589793238462643383279502884197169);
  std::cout << "a[" << a.size() - 1 << "] = " << a[a.size() - 1] << std::endl; // a[2] = 3.14159

  auto b = MakeFilled<std::vector<double>>(5, 3.141592653589793238462643383279502884197169);
  std::cout << "b[" << b.size() - 1 << "] = " << b[b.size() - 1] << std::endl; // b[4] = 3.14159
}

or using std::fill_n.

Or instead of unsigned int VSize some sort of variadic template parameter pack.

[N-Dekker]

We could support variable-sized arrays as well.

Indeed, nice suggestion, but would it be useful enough? For std::vector I think I would still prefer to simply pass the size and the initial "fill value" to one of its constructors:

std::vector<double> v(5, 3.141592653589793238462643383279502884197169);

Do you have other variable-sized container types in mind for which you would want to use MakeFilled?

[N-Dekker]

@Leengit I'm sorry to kill your darlings, and I think std::remove_reference_t<decltype(*TContainer{}.begin())> looks really clever, but for the sake of code readability, I think it would be better to declare the value argument simply by TContainer::const_reference. This const_reference is defined for itk::FixedArray, itk::Index, itk::Offset, and itk::Size, as well as for std::array. OK?

[N-Dekker]

Pull request #3236 aims to pave the way for making MakeFilled constexpr 😃

[Leengit]

Do you have other variable-sized container types in mind for which you would want to use MakeFilled?

The motivation is that someone who prefers our new MakeFilled may want to use it more widely / consistently. There are other ways to accomplish any of this, so it is not a show stopper in any sense. Please proceed as you see fit.

[Leengit]

Well, clang and msvc also fail to compile this

Right you are. The GCC folks indicate that the current behavior is the proper behavior. If we instead want the static_assert to not be checked at compile time in the presence of SFINAE in the code then we have to make the SFINAE present in the static_assert statement itself. That is, static_assert(false, ...) won't do what I want but if false is changed to something that one of the template parameters will always fail then all is good.

[Leengit]

Putting it all together, I think we've arrived at:

template <typename TContainer>
TContainer
MakeFilled(typename TContainer::const_reference & value)
{
  static_assert(TContainer{}.size() > 0, "MakeFilled requires TContainer with constant non-zero size!");
  TContainer result;
  std::fill(result.begin(), result.end(), value);
  return result;
}

or something similar with std::fill_n.

[N-Dekker]

Putting it all together, I think we've arrived at

Yes, thanks @Leengit But now I think it would be even better if we could declare it constexpr! Unfortunately std::fill and std::fill_n only seem to be constexpr from C++20: https://en.cppreference.com/w/cpp/algorithm/fill (ITK still supports C++14.) So then I guess we'd have to have a "hand-written" for loop instead:

template <typename TContainer>
constexpr TContainer
MakeFilled(typename TContainer::const_reference & value)
{
  // Note: The seemingly redundant {} initialization is required for C++14, in order to declare MakeFilled constexpr.
  TContainer container{};

  // MakeFilled requires TContainer with constant size!
  constexpr size_t numberOfElements = container.size();

  static_assert(numberOfElements > 0, "MakeFilled requires TContainer with non-zero size!");

  for (size_t i = 0; i < numberOfElements; ++i)
  {
    container[i] = value;
  }
  return container;
}

What do you think?

[Leengit]

(edited to add comments and make it compile)

Sure. Or as

template <typename TContainer>
constexpr TContainer
MakeFilled(typename TContainer::const_reference & value)
{
  static_assert(TContainer{}.size() > 0, "MakeFilled requires TContainer with constant non-zero size!");

  // Note: The seemingly redundant {} initialization is required for C++14, in order to declare MakeFilled constexpr.
  TContainer container{};
  // Note that `std::fill` and `std::fill_n` are not `constexpr` until C++20.
  for (auto & iter : container)
  {
    iter = value;
  }
  return container;
}

That way, the failure when, e.g., std::vector is used, will be at the static_assert statement rather than at the initialization of numberOfElements.

[N-Dekker]

@Leengit Honestly, I was also thinking of using a range-based for loop, that's why I proposed to declare the begin() and end() of FixedArray, Index, Offset, and Size constexpr (PR #3236 commit 47bce264791a53853020a3911dd94d060cfbd655). But then I saw that std::array begin() and end() are only constexpr from C++17! Fortunately std::array::operator[] and std::array::size() are already constexpr with C++14: https://en.cppreference.com/w/cpp/container/array

Do you agree that it would be useful for MakeFilled to support C++14 std::array, as well as ITK's FixedArray, Index, Offset, and Size?

BTW, if you want the failure on std::vector only at the static_assert, and not at the initialization of numberOfElements, I think we could simply declare numberOfElements as const (rather than constexpr):

const size_t numberOfElements = container.size();
static_assert(numberOfElements > 0, "MakeFilled requires TContainer with constant non-zero size!");

Would you like that better?

[Leengit]

That is a good catch about C++14 vs. C++17. We definitely want this to work with C++14.

Yes, we could do the const instead of constexpr thing because the performance change (e.g., when MakeFilled is called with a value that is computed at runtime) will be minuscule if any. However, I am thinking that we don't need to bother. Even if the static_assert redundantly queries the size(), that's only at compile time, so it doesn't matter. Regardless, we are talking minutia here, so what ever calls to you works for me.

[N-Dekker]

@Leengit Thanks! So... do you want to make it a pull request, or do you want me to do it?

[Leengit]

You may have the honors.

[N-Dekker]

@Leengit Sorry, I was mistaken, when I wrote:

Fortunately std::array::operator[] and std::array::size() are already constexpr with C++14: https://en.cppreference.com/w/cpp/container/array

The non-const std::array::operator[] overload is only constexpr from C++17. But by then, its begin() and end() are also constexpr! So with C++14, itk::MakeFilled<std::array<T, N>>(val) just cannot be called in a constexpr context.

Which implies that I was wrong, when I rejected your range-based for loop suggestion!!! 🐱

[Leengit]

So do we write MakeFilled as if for C++17 and have MakeFilled declared as constexpr only #if __cplusplus >= 201703L?

[N-Dekker]

@Leengit No, it will be unconditionally declared as constexpr 😃 It will work fine outside constexpr context, for std::array, as well as for ITK containers. But within constexpr context, it can only be used for ITK containers (assuming C++14 compilation).