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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

This is going to need some tests... And because you're adding templates to the SBAPI we'll need multiple tests -- Not just on the python side but I think we should add some actual C++ tests here in `test/API/api/`



================
Comment at: lldb/include/lldb/API/SBStructuredData.h:12-13
 
+#include "lldb/Core/StructuredDataImpl.h"
+
 #include "lldb/API/SBDefines.h"
----------------
You can't include a private header in a public header.


================
Comment at: lldb/include/lldb/API/SBStructuredData.h:70-77
+  template <typename T> T GetIntegerValue(T fail_value = {}) const {
+    if constexpr (!std::is_integral_v<T>)
+      return fail_value;
+
+    return m_impl_up->GetType() == eStructuredDataTypeSignedInteger
+               ? m_impl_up->GetIntegerValue(static_cast<int64_t>(fail_value))
+               : m_impl_up->GetIntegerValue(static_cast<uint64_t>(fail_value));
----------------
> All the SB API classes are non-virtual, single inheritance classes. They should only include SBDefines.h or other SB headers as needed. **There should be no inlined method implementations in the header files, they should all be in the implementation files**. And there should be no direct ivar access.

Emphasis mine, from: https://lldb.llvm.org/design/sbapi.html

We should be able to move this into the implementation file.


================
Comment at: lldb/include/lldb/API/SBStructuredData.h:71-72
+  template <typename T> T GetIntegerValue(T fail_value = {}) const {
+    if constexpr (!std::is_integral_v<T>)
+      return fail_value;
+
----------------
This implementation allows us to generate `GetIntegerValue` for things like `const char *` and will just return a `fail_value` for those things. I think the interface would be better if it could give users feedback when they accidentally do something wrong with types instead of just giving them back whatever they want the failure value to be. Is there a way we can use SFINAE to **only** generate `GetIntegerValue` for things that are integral type? Would such a thing be compatible with SWIG?


================
Comment at: lldb/include/lldb/Core/StructuredDataImpl.h:131
 
   uint64_t GetIntegerValue(uint64_t fail_value = 0) const {
+    return (m_data_sp ? m_data_sp->GetUnsignedIntegerValue(fail_value)
----------------
Not related to your change, we should probably move StructuredDataImpl to Utility :P


================
Comment at: lldb/include/lldb/Utility/StructuredData.h:308-309
+    template <typename T> void AddIntegerItem(T value) {
+      static_assert(std::is_integral<T>::value,
+                    "value type should be integral");
+      if constexpr (std::numeric_limits<T>::is_signed)
----------------
Check for floating type here in addition, just like below.


================
Comment at: lldb/include/lldb/lldb-enumerations.h:816-817
   eStructuredDataTypeInteger,
+  eStructuredDataTypeUnsignedInteger = eStructuredDataTypeInteger,
+  eStructuredDataTypeSignedInteger,
   eStructuredDataTypeFloat,
----------------
Is it a good idea to insert these in the middle? Not that people should do this, but I assume some people hardcode these values in their script....


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150485