[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

LGTM modulo comments



================
Comment at: lldb/include/lldb/Target/RegisterFlags.h:21-22
+  public:
+    Field(llvm::StringRef name, unsigned start, unsigned end)
+        : m_name(name.str()), m_start(start), m_end(end) {
+      assert(m_start <= m_end && "Start bit must be <= end bit.");
----------------
If you're going to store the string anyway, you might as well take it by value and move it into the member. That'll save you a copy in case someone calls the ctor with an rvalue reference. 


================
Comment at: lldb/include/lldb/Target/RegisterFlags.h:26
+
+    // Get size of the field in bits. Will always be at least 1.
+    unsigned GetSizeInBits() const { return m_end - m_start + 1; }
----------------
This applies to the rest of this file as well.


================
Comment at: lldb/include/lldb/Target/RegisterFlags.h:72
+  // Gaps are allowed, they will be filled with anonymous padding fields.
+  RegisterFlags(llvm::StringRef id, unsigned size,
+                const std::vector<Field> &fields);
----------------
Same here


================
Comment at: lldb/include/lldb/Target/RegisterFlags.h:81-83
+  std::string m_id;
+  // Size in bytes
+  unsigned m_size;
----------------
Looks like these can be const?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145566