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
