Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,9 @@ FILE: ../../../flutter/fml/dart/dart_converter.h
FILE: ../../../flutter/fml/delayed_task.cc
FILE: ../../../flutter/fml/delayed_task.h
FILE: ../../../flutter/fml/eintr_wrapper.h
FILE: ../../../flutter/fml/endianness.cc
FILE: ../../../flutter/fml/endianness.h
Copy link
Member

Choose a reason for hiding this comment

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

Please add an empty endianness.cc that only include endianness.h. That is a good way to check that the header can always be included cleanly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

FILE: ../../../flutter/fml/endianness_unittests.cc
FILE: ../../../flutter/fml/file.cc
FILE: ../../../flutter/fml/file.h
FILE: ../../../flutter/fml/file_unittest.cc
Expand Down
3 changes: 3 additions & 0 deletions fml/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ source_set("fml") {
"delayed_task.cc",
"delayed_task.h",
"eintr_wrapper.h",
"endianness.cc",
"endianness.h",
"file.cc",
"file.h",
"hash_combine.h",
Expand Down Expand Up @@ -268,6 +270,7 @@ if (enable_unittests) {
"backtrace_unittests.cc",
"base32_unittest.cc",
"command_line_unittest.cc",
"endianness_unittests.cc",
"file_unittest.cc",
"hash_combine_unittests.cc",
"hex_codec_unittest.cc",
Expand Down
5 changes: 5 additions & 0 deletions fml/endianness.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "flutter/fml/endianness.h"
73 changes: 73 additions & 0 deletions fml/endianness.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef FLUTTER_FML_ENDIANNESS_H_
#define FLUTTER_FML_ENDIANNESS_H_

#include <cstdint>
#include <type_traits>
#if defined(_MSC_VER)
#include "intrin.h"
#endif

#include "flutter/fml/build_config.h"

// Compiler intrinsics for flipping endianness.
#define FML_BYTESWAP_16(n) __builtin_bswap16(n)
#define FML_BYTESWAP_32(n) __builtin_bswap32(n)
#define FML_BYTESWAP_64(n) __builtin_bswap64(n)

#if defined(_MSC_VER)
#define FML_BYTESWAP_16(n) _byteswap_ushort(n)
#define FML_BYTESWAP_32(n) _byteswap_ulong(n)
#define FML_BYTESWAP_64(n) _byteswap_uint64(n)
#endif

namespace fml {

/// @brief Flips the endianness of the given value.
/// The given value must be an integral type of size 1, 2, 4, or 8.
template <typename T, class = std::enable_if_t<std::is_integral_v<T>>>
constexpr T ByteSwap(T n) {
Copy link
Member

@chinmaygarde chinmaygarde Jan 27, 2022

Choose a reason for hiding this comment

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

When using templating in C++, please be mindful of the error messages being generated. For instance, if I say ByteSwap("Hello"), the compiler will indicate an error in the header and not the location of incorrect call. Admittedly, there is a note that indicates what led to the error but the user needs to dig deeper to figure out whats going on and the first spot the compiler points you to as being the error is not actually the error.
Screen Shot 2022-01-27 at 2 17 17 PM

One way to avoid this in the scheme you have devised would be to have restrictions on specialization. So, something like , class = std::enable_if_t<std::is_integral<T>::value>.

This makes the error move to the right spot.
Screen Shot 2022-01-27 at 2 21 15 PM

The compiler note will indicate why the specialization failed. Here, the error is in the right spot and the note indicates what the user must to do make the specialization work.

BTW, the restriction may even avoid you needing the check for other zero sized typed and the whole static assert can be false.

In case you want a link to the sandbox where you can experiment with templating and error messages.

Another suggestion would be to just add explicit overloads for each integral type. I suppose that would work for ByteSwap but get tedious for <Foo>EndianFrom/ToArch. So, your call on that suggestion.

Templating is really interesting and powerful but its easy to call it a day without paying sufficient attention to UX concerns around its use which gives the technique a worse rep than it perhaps deserves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting! I haven't touched adding type restrictions like this, so this was illuminating. I added what you suggested here, but if you have a preference for adding overloads in this case, let me know and I'll do it (I don't have a strong opinion either way).

if constexpr (sizeof(T) == 1) {
return n;
} else if constexpr (sizeof(T) == 2) {
return (T)FML_BYTESWAP_16((uint16_t)n);
} else if constexpr (sizeof(T) == 4) {
return (T)FML_BYTESWAP_32((uint32_t)n);
} else if constexpr (sizeof(T) == 8) {
return (T)FML_BYTESWAP_64((uint64_t)n);
} else {
static_assert(!sizeof(T), "Unsupported size");
}
}

/// @brief Convert a known big endian value to match the endianness of the
/// current architecture. This is effectively a cross platform
/// ntohl/ntohs (as network byte order is always Big Endian).
/// The given value must be an integral type of size 1, 2, 4, or 8.
template <typename T, class = std::enable_if_t<std::is_integral_v<T>>>
constexpr T BigEndianToArch(T n) {
#if ARCH_CPU_LITTLE_ENDIAN
Copy link
Member

Choose a reason for hiding this comment

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

Huh. I didn't realize we had un-"namespaced" macros in fml/build_config

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow-up patch incoming :)

Copy link
Member Author

Choose a reason for hiding this comment

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

return ByteSwap<T>(n);
#else
return n;
#endif
}

/// @brief Convert a known little endian value to match the endianness of the
/// current architecture.
/// The given value must be an integral type of size 1, 2, 4, or 8.
template <typename T, class = std::enable_if_t<std::is_integral_v<T>>>
constexpr T LittleEndianToArch(T n) {
#if !ARCH_CPU_LITTLE_ENDIAN
return ByteSwap<T>(n);
#else
return n;
#endif
}

} // namespace fml

#endif // FLUTTER_FML_ENDIANNESS_H_
37 changes: 37 additions & 0 deletions fml/endianness_unittests.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "flutter/fml/endianness.h"

#include "flutter/testing/testing.h"

namespace fml {
namespace testing {

TEST(EndiannessTest, ByteSwap) {
ASSERT_EQ(ByteSwap<int16_t>(0x1122), 0x2211);
ASSERT_EQ(ByteSwap<int32_t>(0x11223344), 0x44332211);
ASSERT_EQ(ByteSwap<uint64_t>(0x1122334455667788), 0x8877665544332211);
}

TEST(EndiannessTest, BigEndianToArch) {
#if ARCH_CPU_LITTLE_ENDIAN
uint32_t expected = 0x44332211;
#else
uint32_t expected = 0x11223344;
#endif
ASSERT_EQ(BigEndianToArch(0x11223344u), expected);
}

TEST(EndiannessTest, LittleEndianToArch) {
#if ARCH_CPU_LITTLE_ENDIAN
uint32_t expected = 0x11223344;
#else
uint32_t expected = 0x44332211;
#endif
ASSERT_EQ(LittleEndianToArch(0x11223344u), expected);
}

} // namespace testing
} // namespace fml