[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:818
+      unsigned encode() const {
+        return ((unsigned)HasReturn) | (HasTailCall << 1) | (IsEHPad << 2) |
+               (CanFallThrough << 3);
----------------
Nit: use `static_cast<unsigned>`


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:820
+               (CanFallThrough << 3);
+      }
+      // Decodes and returns a Metadata struct from an unsigned integer value.
----------------
Nit: blank line after method.


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:822
+      // Decodes and returns a Metadata struct from an unsigned integer value.
+      static Metadata decode(unsigned V) {
+        return Metadata{/*HasReturn=*/static_cast<bool>(V & 1),
----------------
Could you add some simple unit tests for the encode/decode methods, please? They should be trivial to write, but I think would just help ensure there are no surprises. I think it would be sufficient to cover all 4 fields set individually, and also all fields set and no fields set.


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:828
+      }
+    } MD;
+
----------------
FWIW, I'd find it more readable if you declared the struct, and then declared the member on a separate line i.e:
```
struct Metadata {
  ...
};

Metadata MD;
```


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:830-833
+    bool hasReturn() const { return MD.HasReturn; }
+    bool hasTailCall() const { return MD.HasTailCall; }
+    bool isEHPad() const { return MD.IsEHPad; }
+    bool canFallThrough() const { return MD.CanFallThrough; }
----------------
I'm not sure these should be moved. Typically, the constructor comes before other methods, from what I've seen, and it would reduce the size of the diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148360