PERF: Use FixedArray for table within BSplineInterpolationWeightFunction

[N-Dekker]

Replaced itk::Array2D by itk::FixedArray, used as private TableType
of BSplineInterpolationWeightFunction. This is the type of a table,
used internally by its Evaluate member function.

Declared this table const, and initialized the table by an in-class
default-member-initializer. Defaulted the default-constructor.

Follow-up to pull request #2712
commit bc7c5df
"PERF: Use FixedArray for BSplineBaseTransform ParameterIndexArrayType"
commit 9bf745b
"PERF: Use FixedArray for BSplineInterpolationWeightFunction OutputType"

[dzenanz]

I guess this exposes a need for more follow-up, Modules/Core/Transform/include/itkBSplineTransform.hxx:629:44: error: too many arguments to function call, expected single argument 'index', have 3 arguments

[N-Dekker]

/azp run ITK.Linux

[N-Dekker]

@thewtex Would this PR still be in time for ITK 5.3? It would be nice to have it included in the same release as the other two BSpline "Use FixedArray for ..." commits, from pull request #2712, as they kinda belong together 😃

[dzenanz]

There is a greater chance of that if you merge it now, or as soon as the CI passes 😄

[N-Dekker]

There is a greater chance of that if you merge it now, or as soon as the CI passes 😄

Thanks for the suggestion @dzenanz As you know I'd rather not merge my own PRs. But maybe, if this one gets one more approval, I'll just do it. 😸

@Leengit I feel your comment #2721 (comment) is relevant, but it's not a show-stopper to this particular PR. OK?

[Leengit]

@Leengit I feel your comment #2721 (comment) is relevant, but it's not a show-stopper to this particular PR. OK?

Not a show stopper. Don't wait on account of me!

[dzenanz]

I now see that Matt has tagged 5.3 RC1. This PR didn't make the cut 😞

[N-Dekker]

I now see that Matt has tagged 5.3 RC1. This PR didn't make the cut 😞

@dzenanz @thewtex So sad for this lonely little PR to be apart from the other two BSpline "Use FixedArray for ..." commits 😢 Any chance that RC2 will bring them together again?

[dzenanz]

RC2 will be cut from master, so they will be reunited 💕

[thewtex]

Yes, 5.3 RC 2 will include these. Before then, there are new dashboard errors to address https://open.cdash.org/viewBuildError.php?buildid=7449043 😸

[Leengit]

If it isn't making sense to defined SupportSize outside the class declaration so that references and pointers to it are enabled -- perhaps because we don't want to allocate that memory just to support the reference -- similar can be accomplished via a local variable to this lambda.

const auto supportSize {Self::SupportSize};

As to const vs. constexpr here, I think the former might work out better -- the latter might be interpreted as a hint to the compiler that we don't want the memory allocated.

[N-Dekker]

@Leengit Indeed, it appears that the link error could also be avoided by a local const auto supportSize within the lambda. But personally I don't really like to have an extra local variable. For now I think the syntax SizeType{ SupportSize } is just fine.

In general, it might become cumbersome to spell out the type name of the constexpr data member every time it is passed by reference. Instead maybe a helper function could be useful:

auto CopyToNonConstexprValue(T value) { return value; }

The value of the constexpr SupportSize could then be passed to a function like ProcessSize(const SizeType&), as follows:

ProcessSize( CopyToNonConstexprValue(SupportSize) );

Do you like that? 😺

[Leengit]

I fear that it will give the same errors that we are already / still seeing in cdash. But if it doesn't then I like it.

[N-Dekker]

@Leengit Try it out at https://godbolt.org/z/YGTh9q1Po 😃 As follows (using GCC 10.3):

struct SizeType {};

struct Wrapper
{
  constexpr static SizeType ConstexprValue{};
};

void ProcessSize(const SizeType&) {}

template <typename T>
auto CopyToNonConstexprValue(T value) { return value; }

int main()
{
    ProcessSize(CopyToNonConstexprValue(Wrapper::ConstexprValue)); // OK
    ProcessSize(Wrapper::ConstexprValue); // <== error: undefined reference
}

[N-Dekker]

@Leengit Related section from https://github.com/cplusplus/draft/releases/download/n4892/n4892.pdf (C++ Working Draft, 2021-06-18):

D.7 Redeclaration of static constexpr data members [depr.static.constexpr]

1 For compatibility with prior revisions of C++, a constexpr static data member may be redundantly redeclared
outside the class with no initializer. This usage is deprecated.
[Example 1:

struct A {
  static constexpr int n = 5; // definition (declaration in C++2014)
};
constexpr int A::n; // redundant declaration (definition in C++2014)

— end example]

[Leengit]

Looks like a good idea. Is this for C++23? Do they have an alternative way of making references and pointers work for class static constexpr members, or are they saying that the class declaration needs to be sufficient?

[Leengit]

I didn't try your CopyToNonConstexprValue example but ... looks like it works!

[Leengit]

I didn't try your CopyToNonConstexprValue example but ... looks like it works!

Like the use of a local variable, this will require the existence of a (implicit or explicit) copy constructor. The deprecated technique of redundantly declaring the member variable outside of the class declaration allows any constructor.