Add constexpr endianness utilities to fml #31118
Conversation
| FILE: ../../../flutter/fml/delayed_task.cc | ||
| FILE: ../../../flutter/fml/delayed_task.h | ||
| FILE: ../../../flutter/fml/eintr_wrapper.h | ||
| FILE: ../../../flutter/fml/endianness.h |
There was a problem hiding this comment.
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.
fml/endianness.h
Outdated
| #include "flutter/fml/build_config.h" | ||
|
|
||
| // Compiler intrinsics for flipping endianness. | ||
| #define BYTESWAP16(n) __builtin_bswap16(n) |
There was a problem hiding this comment.
Please "namespace" macros in FML with the FML_ prefix so we don't run into macro collisions. So, FML_BYTESWAP_BLAH.
| /// ntohl/ntohs (as network byte order is always Big Endian). | ||
| template <typename T> | ||
| constexpr T BigEndianToArch(T n) { | ||
| #if ARCH_CPU_LITTLE_ENDIAN |
There was a problem hiding this comment.
Huh. I didn't realize we had un-"namespaced" macros in fml/build_config
There was a problem hiding this comment.
Follow-up patch incoming :)
|
|
||
| /// @brief Flips the endianness of the given value. | ||
| template <typename T> | ||
| constexpr T ByteSwap(T n) { |
There was a problem hiding this comment.
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.
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.
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.
There was a problem hiding this comment.
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).
bdero
force-pushed
the
bdero/endianness
branch
from
96cadec to
89b42b8
Compare
bdero
force-pushed
the
bdero/endianness
branch
from
89b42b8 to
e85b19d
Compare
* 7b26f7a Roll Skia from 32f7fa7bfef3 to c4ba48a7d8c3 (2 revisions) (flutter/engine#31139) * 312551e Revert "Revert engine (#31135)" (flutter/engine#31142) * e17ee4a Add constexpr endianness utilities to fml (flutter/engine#31118) * 4b55492 Remove unused Counter and CounterValues class. (flutter/engine#31130)
* 7b26f7a Roll Skia from 32f7fa7bfef3 to c4ba48a7d8c3 (2 revisions) (flutter/engine#31139) * 312551e Revert "Revert engine (flutter#31135)" (flutter/engine#31142) * e17ee4a Add constexpr endianness utilities to fml (flutter/engine#31118) * 4b55492 Remove unused Counter and CounterValues class. (flutter/engine#31130)
2 participants
Pulled this out of the WIP APNG demuxer: #31098
Uses byteswap compiler intrinsics. I think all the architectures we currently target are little endian, but this is convenient for parsing older big endian formats (like PNG chunks).