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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to