[Lldb-commits] [PATCH] D33434: Added new API to SBStructuredData class

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Very close and overall very nice, just a few implementation details to take care of.



================
Comment at: include/lldb/API/SBStructuredData.h:20-28
+  enum Type {
+    eTypeInvalid = -1,
+    eTypeArray = 0,
+    eTypeInteger,
+    eTypeFloat,
+    eTypeBoolean,
+    eTypeString,
----------------
This should go into lldb-enumerations.h and the lldb_private::StructuredData should switch over to using that enum as well. Then we won't duplicated the enum here and in StructuredData.h. The enum type name will need to become longer and the enums will need to include the prefix be longer:

```
enum StructureDataType {
  eStructureDataTypeInvalid,
  eStructureDataTypeArray,
  ...
```



================
Comment at: include/lldb/API/SBStructuredData.h:53
+  //------------------------------------------------------------------
+  lldb::SBStructuredData::Type GetType() const;
+
----------------
lldb::StructureDataType to match the new enum in lldb-enumerations.h


================
Comment at: include/lldb/API/SBStructuredData.h:60
+  //------------------------------------------------------------------
+  size_t GetSize() const;
+
----------------
What is the user going to really do when getting the size from a dictionary? I would vote to just make this for arrays and provide a IsEmpty() method if you want to check if the dictionary or array is empty? I am open for reasons why we should be able to get the size of the dictionary if there is a need, I am just thinking out loud here.


================
Comment at: include/lldb/API/SBStructuredData.h:93
+  //------------------------------------------------------------------
+  size_t GetStringValue(char *dst, size_t dst_len) const;
+
----------------
We should make this be able to take NULL for dst and/or zero for dst_len and have it return the size that would be needed to store the value. Basically document:
- the return value is always the byte size needed for the string value
- if the buffer supplied is insufficient, then dst_len -1 bytes will be filled in and the string will be NULL terminated prematurely
- the if the buffer supplied is invalid (dst == NULL, or dst_len == 0), then no data will be filled in


================
Comment at: include/lldb/API/SBStructuredData.h:98-99
 
+  SBStructuredData(const lldb::StructuredDataImplUP &impl_up);
+
   StructuredDataImplUP m_impl_up;
----------------
Remove this.


================
Comment at: include/lldb/Core/StructuredDataImpl.h:99-103
+    if (m_data_sp->GetType() == StructuredData::Type::eTypeDictionary) {
+      StructuredData::Dictionary *dict =
+          static_cast<lldb_private::StructuredData::Dictionary *>(
+              m_data_sp.get());
+      return (dict->GetSize());
----------------
Do we need to get the size for a dictionary? See above inlined comment.


================
Comment at: include/lldb/Core/StructuredDataImpl.h:113-120
+    if (!m_data_sp ||
+        (m_data_sp->GetType() != StructuredData::Type::eTypeDictionary))
+      return StructuredData::ObjectSP();
+
+    StructuredData::Dictionary *dict =
+        static_cast<lldb_private::StructuredData::Dictionary *>(
+            m_data_sp.get());
----------------
Might be easier to use the StructuredData::GetAsDictionary():
```
if (m_data_sp) {
  auto dict = m_data_sp->GetAsDictionary();
  if (dict)
    return dict->GetValueForKey(llvm::StringRef(key))
}
return StructuredData::ObjectSP();
```


================
Comment at: include/lldb/Core/StructuredDataImpl.h:124-129
+    if (!m_data_sp ||
+        (m_data_sp->GetType() != StructuredData::Type::eTypeArray))
+      return StructuredData::ObjectSP();
+
+    StructuredData::Array *array =
+        static_cast<lldb_private::StructuredData::Array *>(m_data_sp.get());
----------------
Use GetAsArray() like above dictionary requested changes.


================
Comment at: scripts/interface/SBStructuredData.i:22-30
+        enum Type {
+          eTypeInvalid = -1,
+          eTypeArray = 0,
+          eTypeInteger,
+          eTypeFloat,
+          eTypeBoolean,
+          eTypeString,
----------------
Remove and use from lldb-enumerations.h


================
Comment at: source/API/SBStructuredData.cpp:34-35
 
+SBStructuredData::SBStructuredData(const lldb::StructuredDataImplUP &impl_up)
+    : m_impl_up(new StructuredDataImpl(*(impl_up.get()))) {}
+
----------------
Remove this.


================
Comment at: source/API/SBStructuredData.cpp:114
+
+  StructuredDataImplUP impl_up(new StructuredDataImpl());
+  impl_up->SetObjectSP(m_impl_up->GetValueForKey(key));
----------------
Just default construct one:

```
SBStructuredData result;
result.m_impl_up->SetObjectSP(m_impl_up->GetValueForKey(key));
return result;
```


================
Comment at: source/API/SBStructuredData.cpp:123-125
+  StructuredDataImplUP impl_up(new StructuredDataImpl());
+  impl_up->SetObjectSP(m_impl_up->GetItemAtIndex(idx));
+  return lldb::SBStructuredData(impl_up);
----------------
Default construct and use the object:

```
SBStructuredData result;
result.m_impl_up->SetObjectSP(m_impl_up-> GetItemAtIndex(idx));
return result;
```


https://reviews.llvm.org/D33434