[Lldb-commits] [PATCH] D150485: [lldb] Add support for negative integer to {SB, }StructuredData

Mon May 22 13:16:01 PDT 2023

bulbazord added a comment.

Ok, this is a pretty big patch so we probably don't want to leave it in review for a long time or it just gets harder to land correctly.

I've looked over most of this patch and it looks fine to me. A lot of is rather mechanical: Changing things like `GetIntegerValue` to `GetUnsignedIntegerValue` and using `GetSignedIntegerValue` where it makes sense. There are lots of little cleanups as well, like correcting the type of some variables (e.g. `addr_t` -> `offset_t`) and these are fine too. I don't have much of a problem with any of that.

2 small things though and I think this should probably be good after that.

================
Comment at: lldb/include/lldb/API/SBStructuredData.h:75-78
+  //  LLDB_DEPRECATED("Specify if the value is signed or unsigned",
+  //                  "uint64_t GetUnsignedIntegerValue(uint64_t fail_value =
+  //                  0)")
+  [[deprecated("Specify if the value is signed or unsigned")]] uint64_t
----------------
Did you mean to remove the deprecated attribute and use `LLDB_DEPRECATED` directly?

================
Comment at: lldb/include/lldb/Utility/StructuredData.h:105-106
+    UnsignedInteger *GetAsUnsignedInteger() {
+      return ((m_type == lldb::eStructuredDataTypeInteger ||
+               m_type == lldb::eStructuredDataTypeUnsignedInteger)
+                  ? static_cast<UnsignedInteger *>(this)
----------------
Could you add a small note here specifying why `eStructuredDataTypeInteger` corresponds to UnsignedInteger and not SignedInteger?

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

https://reviews.llvm.org/D150485