[PATCH] D148360: [Propeller] Use a bit-field struct for the metdata fields of BBEntry.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:807-811
+      bool operator==(const Metadata &Other) const {
+        return HasReturn == Other.HasReturn &&
+               HasTailCall == Other.HasTailCall && IsEHPad == Other.IsEHPad &&
+               CanFallThrough == Other.CanFallThrough;
+      }
----------------
Nit: I'd add blank lines either side of this method.


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:813
+      // Encodes this struct as an unsigned integer.
+      unsigned encode() const {
+        return static_cast<unsigned>(HasReturn) |
----------------
Thinking about it, it's probably more appropriate for this to return a fixed width type (i.e. uint32_t) and similarly for the casts to cast to that.

Strictly speaking, I don't think there's any guarantee that unsigned is 4 bytes in size, plus it expresses the intent a little better in my opinion.


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:821
+      // Decodes and returns a Metadata struct from an unsigned integer value.
+      static Metadata decode(unsigned V) {
+        return Metadata{/*HasReturn=*/static_cast<bool>(V & 1),
----------------
As above, uint32_t would be more appropriate here.


================
Comment at: llvm/unittests/Object/ELFTypesTest.cpp:66
+TEST(ELFTypesTest, BBEntryMetadataEncodingTest) {
+  static const std::array<BBAddrMap::BBEntry::Metadata, 6> Decoded = {
+      {{false, false, false, false},
----------------
It might be interesting to show what happens when you provide `decode` with a value that includes bits that aren't related to any of the properties. For example, 0xffffffff.


================
Comment at: llvm/unittests/Object/ELFTypesTest.cpp:73
+       {true, true, true, true}}};
+  static const std::array<unsigned, 6> Encoded = {{0, 1, 2, 4, 8, 15}};
+  for (unsigned i = 0; i < Decoded.size(); ++i)
----------------
What's the reason for these two being `static`? It's not modifiable, so I don't think it benefits anything, since it's already function-local.


================
Comment at: llvm/unittests/Object/ELFTypesTest.cpp:74
+  static const std::array<unsigned, 6> Encoded = {{0, 1, 2, 4, 8, 15}};
+  for (unsigned i = 0; i < Decoded.size(); ++i)
+    EXPECT_EQ(Decoded[i].encode(), Encoded[i]);
----------------
These two loops should use `size_t` since that's what `size` returns.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148360/new/

https://reviews.llvm.org/D148360