Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

STYLE: Clean-up ByteSwapper, using private SwapBytes helper function #4855

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

STYLE: Clean-up ByteSwapper, using private SwapBytes helper function #4855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/itk_dict.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ btw
buf
bugfix
bw
byteswap
byu
bzip
calc
Expand Down
5 changes: 5 additions & 0 deletions Modules/Core/Common/include/itkByteSwapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,11 @@ class ITK_TEMPLATE_EXPORT ByteSwapper : public Object
SwapWrite8Range(const void * ptr, BufferSizeType num, OStreamType * fp);

private:
/** Swaps the bytes of the specified argument in-place. Assumes that its number of bytes is either 2, 4, or 8.
* Otherwise, it throws an exception. */
static void
SwapBytes(T &);

static constexpr bool m_SystemIsBigEndian{
#ifdef CMAKE_WORDS_BIGENDIAN
true
Expand Down
106 changes: 37 additions & 69 deletions Modules/Core/Common/include/itkByteSwapper.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@
#ifndef itkByteSwapper_hxx
#define itkByteSwapper_hxx
#include "itkMakeUniqueForOverwrite.h"
#include <algorithm> // For swap.
#include <algorithm> // For for_each_n and swap.
#include <cstddef> // For byte.
#include <memory>
#include <cstring>

Expand All @@ -44,49 +45,19 @@ template <typename T>
void
ByteSwapper<T>::SwapFromSystemToBigEndian([[maybe_unused]] T * p)
{
if constexpr (!m_SystemIsBigEndian)
if constexpr (!m_SystemIsBigEndian && sizeof(T) > 1)
{
switch (sizeof(T))
{
case 1:
return;
case 2:
Self::Swap2(p);
return;
case 4:
Self::Swap4(p);
return;
case 8:
Self::Swap8(p);
return;
default:
itkGenericExceptionMacro("Cannot swap number of bytes requested");
}
SwapBytes(*p);
}
}

template <typename T>
void
ByteSwapper<T>::SwapRangeFromSystemToBigEndian([[maybe_unused]] T * p, [[maybe_unused]] BufferSizeType num)
{
if constexpr (!m_SystemIsBigEndian)
if constexpr (!m_SystemIsBigEndian && sizeof(T) > 1)
{
switch (sizeof(T))
{
case 1:
return;
case 2:
Self::Swap2Range(p, num);
return;
case 4:
Self::Swap4Range(p, num);
return;
case 8:
Self::Swap8Range(p, num);
return;
default:
itkGenericExceptionMacro("Cannot swap number of bytes requested");
}
std::for_each_n(p, num, [](T & element) { SwapBytes(element); });
N-Dekker marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -127,49 +98,19 @@ template <typename T>
void
ByteSwapper<T>::SwapFromSystemToLittleEndian([[maybe_unused]] T * p)
{
if constexpr (m_SystemIsBigEndian)
if constexpr (m_SystemIsBigEndian && sizeof(T) > 1)
{
switch (sizeof(T))
{
case 1:
return;
case 2:
Self::Swap2(p);
return;
case 4:
Self::Swap4(p);
return;
case 8:
Self::Swap8(p);
return;
default:
itkGenericExceptionMacro("Cannot swap number of bytes requested");
}
SwapBytes(*p);
}
}

template <typename T>
void
ByteSwapper<T>::SwapRangeFromSystemToLittleEndian([[maybe_unused]] T * p, [[maybe_unused]] BufferSizeType num)
{
if constexpr (m_SystemIsBigEndian)
if constexpr (m_SystemIsBigEndian && sizeof(T) > 1)
{
switch (sizeof(T))
{
case 1:
return;
case 2:
Self::Swap2Range(p, num);
return;
case 4:
Self::Swap4Range(p, num);
return;
case 8:
Self::Swap8Range(p, num);
return;
default:
itkGenericExceptionMacro("Cannot swap number of bytes requested");
}
std::for_each_n(p, num, [](T & element) { SwapBytes(element); });
}
}

Expand Down Expand Up @@ -369,6 +310,33 @@ ByteSwapper<T>::SwapWrite8Range(const void * ptr, BufferSizeType num, OStreamTyp
}
}
}


// The following member function is private:

template <typename T>
void
ByteSwapper<T>::SwapBytes(T & value)
{
static constexpr size_t numberOfBytes = sizeof(T);

// Historically (from ITK v1.2.0, March 2003) the following number of bytes are supported:
if constexpr (numberOfBytes == 2 || numberOfBytes == 4 || numberOfBytes == 8)
{
// When the value is an integer, the following code is equivalent to `value = std::byteswap(value)`, in C++26:
auto * const bytes = reinterpret_cast<std::byte *>(&value);

for (size_t i{}; i < (numberOfBytes / 2); ++i)
{
std::swap(bytes[i], bytes[(numberOfBytes - 1) - i]);
}
}
else
{
itkGenericExceptionMacro("Cannot swap number of bytes requested");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a run-time exception for a "constexpr" conditional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! Only the condition of an if constexpr (condition) { ... } must be evaluated at compile-time. Anything inside the if or else clause may be evaluated at run-time.

Copy link
Contributor Author

@N-Dekker N-Dekker Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this exception, we might also just trigger a compile-time assert failure (static_assert). But obviously, that would change the behavior of ByteSwapper (at least for an unsupported number of bytes). It might be considered an improvement, to replace the exception with a static_assert, of course. Would you like that?

We might just do:

static_assert(numberOfBytes == 2 || numberOfBytes == 4 || numberOfBytes == 8);

Would you like that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I think it would be an improvement and help detect errors in code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly it would be fine to me to also support other number of bytes. Why not support numberOfBytes == 16, for example? For example, when a user wants to swap 128-bit integers...!

I think the original itkGenericExceptionMacro("Cannot swap number of bytes requested") was there just because we only had three of those helper functions: Swap2, Swap4, and Swap8. Instead, we can now support just any number of bytes! Would you like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still broken, now it's itkVTKPolyDataMeshIO, at https://open.cdash.org/viewBuildError.php?type=1&buildid=9915657:

in instantiation of function template specialization 'itk::VTKPolyDataMeshIO::ReadPointsBufferAsBINARY' requested here

  CASE_INVOKE_BY_TYPE(ReadPointsBufferAsBINARY, inputFile)
                      ^

So would you also want us to drop long double support from itk::VTKPolyDataMeshIO?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsurprisingly, this endian swapping only affects IO classes.

As the other float types are being swapped, why not swap long double as well? Just add size of the long double to the list of the allowed sizes in the assert? @blowekamp what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quick google said 128-bit float may be an abnormal case:
https://www.talospace.com/2018/12/the-saga-of-power-isa-128-bit-long.html

So... Just leave it as a run time exception of now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be fine to me to restore the itkGenericExceptionMacro("Cannot swap number of bytes requested") that I originally proposed, and remove the compile-time static_assert(numberOfBytes is 2 or 4 or 8).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should leave support for long double if there is no need for endian swapping (the usual case).

}
}

} // end namespace itk

#endif