[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.

No need for the "StructuredDataType::" or "lldb::StructuredDataType::" prefixes. Since we aren't using an enum class (it was before) and since all enumerators start with "eStructuredDataType", it is now clear where this enum comes from without prefixing with the enum type name and/or namespace. I know you were replicating what was there before. There are two ways to fix this:
1 - define the lldb::StructuredDataType and "enum class StructuredDataType" and then shorten the names by removing eStructuredDataType from each enumerator
2 - leave lldb::StructuredDataType defined as enum StructuredDataType" and leave the names long and they can be used on their own.

I vote for #2 since that is what most other enums do in lldb-enumerations and also for python compatibility. Swig auto creates all enumerations and not sure if it does things correctly for "enum class", so best to go with option #2

So fix the std::string copy and drop all "lldb::StructuredDataType::" or "StructuredDataType::" from the names in the code and this will be good to go.



================
Comment at: include/lldb/API/SBStructuredData.h:50
+  //------------------------------------------------------------------
+  size_t GetSize() const;
+
----------------
No worries, If there is a viable use case, and unit testing is, then we can have this.


================
Comment at: include/lldb/Core/StructuredDataImpl.h:142
+
+    std::string result = m_data_sp->GetStringValue();
+    if (result.empty())
----------------
This comes as a llvm::StringRef. No need to copy the string, just use llvm::StringRef so we don't make a heap copy of the string just to copy it.


https://reviews.llvm.org/D33434