STYLE: Remove std::pow calls from BoundingBox::ComputeCorners()

[N-Dekker]

The use of bit-shift and modulo is a bit more elegant in this case, it may be slightly faster, and it takes away the need to do static_cast's.

The use of std::bitset is a bit clearer in this case, it may be slightly faster than those std::pow calls, and it takes away the need to do static_cast's.

[blowekamp]

I have stared at this statement for a min or two. It looks like its just checking the value of the i'th bit... The following may be clearer:

const bool bit_i = ((1ULL << i) & j);
pnt[i] = center[i] + (bit_i ? -1 :  1 ) * radius[I];

Minimally, I find the current one statement bit too complex for clarity.

[N-Dekker]

Thanks @blowekamp but maybe std::bitset can make it even clearer 🤔

  for (SizeValueType j = 0; j < NumberOfCorners; ++j)
  {
    const std::bitset<PointDimension> bitSet{ j };
    PointType pnt;
    for (unsigned int i = 0; i < PointDimension; ++i)
    {
      pnt[i] = center[i] + (bitSet[i] ? -1 : 1) * radius[i];
    }

    result[j] = pnt;
  }

Instead of the original:

ITK/Modules/Core/Common/include/itkBoundingBox.hxx

Lines 95 to 106 in 0b16b74

	for (SizeValueType j = 0; j < NumberOfCorners; ++j)
	{
	PointType pnt;
	for (unsigned int i = 0; i < PointDimension; ++i)
	{
	pnt[i] = center[i] +
	std::pow(-1.0, (static_cast<double>(j / (static_cast<int>(std::pow(2.0, static_cast<double>(i))))))) *
	radius[i];
	}
	result[j] = pnt;
	}

Would such a std::bitset also be OK to you as well?

[dzenanz]

bitset looks like it was introduced with C++20?

[N-Dekker]

I guess you mean C++98!

[dzenanz]

I saw some "since C+20, since C++23" notes on some functions/constructors.

[N-Dekker]

And in C++, 98 < 20 😄

[N-Dekker]

By the way, I did not yet apply my idea to use std::bitset, because I still think the initial version of my PR is also OK. I feel that it is already a significant improvement to the original, std::pow(-1.0, (static_cast<double>(j / (static_cast<int>(std::pow(2.0, static_cast<double>(i))))))).

Regarding Bradley's (@blowekamp) suggestion, it would be OK to me to use ((1ULL << i) & j), instead of ((j >> i) % 2ULL). Does that already make things clearer to anyone? In general, I prefer not to add variables that are used only once. But if it helps to make people happy, and increases code readability, OK... but then I think it should still be adjusted somewhat. I think it should be camel case, e.g., isBitSet, and it should not silently convert integer to bool. So maybe:

    // Is the i-th bit of j set?
    const bool isBitSet{ ((1ULL << i) & j) != 0 };

What do you think?

• as is
• just replace ((j >> i) % 2ULL) with ((1ULL << i) & j)
• adding an extra local variable, bool isBitSet{ ((1ULL << i) & j) != 0 };
• using std::bitset as I suggested at STYLE: Remove std::pow calls from BoundingBox::ComputeCorners() #5255 (comment)
• or ...?

[blowekamp]

I think your postposed statement is on the complicated side. Your iteration on my suggestion is fine, using a std::bitset maybe better. At minimal a comment it needed to this current proposal IMHO. Many option to improve the PR, thank you for the consideration.

[dzenanz]

Breaking up that statement into two, as Brad suggested, is highly appealing to me. It significantly aids understanding. The other variations are less important to me, with mild preference for your original (using std::bitset) proposal.