[PATCH] D23036: [SelectionDAG] Use a union of bitfield structs for SDNode::SubclassData.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:408-411
@@ +407,6 @@
+
+    bool hasDebugValue() const { return _HasDebugValue; }
+    void setHasDebugValue(bool V) { _HasDebugValue = V; }
+    bool isMemIntrinsic() const { return _IsMemIntrinsic; }
+    void setIsMemIntrinsic(bool V) { _IsMemIntrinsic = V; }
+
----------------
How much are getters and setters at this layer really providing?

If we need these, I'd expect to want them in the actual SDNode public API. For participants in the SDNode type hierarchy, they don't seem to add a lot of value over directly referencing the bitfield.

================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:413-414
@@ +412,4 @@
+
+    // Prefixed with "_" so you're not tempted to use these directly.  Use the
+    // accessors above.
+    uint16_t _HasDebugValue : 1;
----------------
You can't actually do this. An underscore followed by a capital letter is a reserved identifier.

But see above, I don't think we need to do this.

================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:415
@@ +414,3 @@
+    // accessors above.
+    uint16_t _HasDebugValue : 1;
+    uint16_t _IsMemIntrinsic : 1;
----------------
using an explicitly *sized* type for a bitfield seems... actively confusing. I'd just use 'unsigned'.


https://reviews.llvm.org/D23036