This is an archive of the discontinued LLVM Phabricator instance.

Added new API to SBStructuredData class
ClosedPublic

Authored by abhishek.aggarwal on May 23 2017, 3:04 AM.

Details

Summary
  • Added API to access data types
    • integer, double, array, string, boolean and dictionary data types
    • Earlier user had to parse through the string output to get these values
  • Added Test cases for API testing
  • Added new StructuredDataType enum in public include file
    • Replaced locally-defined enum in StructuredData.h with this new one
    • Modified other internal files using this locally-defined enum

Signed-off-by: Abhishek Aggarwal <abhishek.a.aggarwal@intel.com>

Diff Detail

Event Timeline

clayborg requested changes to this revision.May 23 2017, 10:47 AM

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

include/lldb/API/SBStructuredData.h
20–28

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,
  ...
43

lldb::StructureDataType to match the new enum in lldb-enumerations.h

50

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.

83

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
102–103

Remove this.

include/lldb/Core/StructuredDataImpl.h
117–121

Do we need to get the size for a dictionary? See above inlined comment.

131–138

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();
142–147

Use GetAsArray() like above dictionary requested changes.

scripts/interface/SBStructuredData.i
21–29

Remove and use from lldb-enumerations.h

source/API/SBStructuredData.cpp
34–35

Remove this.

112

Just default construct one:

SBStructuredData result;
result.m_impl_up->SetObjectSP(m_impl_up->GetValueForKey(key));
return result;
121–123

Default construct and use the object:

SBStructuredData result;
result.m_impl_up->SetObjectSP(m_impl_up-> GetItemAtIndex(idx));
return result;
This revision now requires changes to proceed.May 23 2017, 10:47 AM
abhishek.aggarwal edited edge metadata.
abhishek.aggarwal marked 9 inline comments as done.

Updating D33434: Added new API to SBStructuredData class

  • Made changes according to feedback

My comments are inlined. Please let me know if something still needs to be changed.

include/lldb/API/SBStructuredData.h
50

I thought of the use case that a user might expect lldb to fill specific number of entries in a dictionary. This might serve as a failure check for the user right in the begining of his code without accessing each (key:value) pair inside it to conclude that enough number of entries are not filled inside the dictionary. I am leaving it as it is for now. However, if you disagree then I will change it according to your proposal.

clayborg requested changes to this revision.May 24 2017, 9:15 AM

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.

include/lldb/API/SBStructuredData.h
50

No worries, If there is a viable use case, and unit testing is, then we can have this.

include/lldb/Core/StructuredDataImpl.h
142

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.

This revision now requires changes to proceed.May 24 2017, 9:15 AM
labath added a subscriber: labath.May 24 2017, 10:13 AM
labath added inline comments.
packages/Python/lldbsuite/test/python_api/sbstructureddata/TestStructuredDataAPI.py
43

Why do you need a process for this test? It sounds like all you're doing is playing around with SBStructuredData. You can remove everything from here including the self.build() call and the inferior source.

Then you can annotate this test with NO_DEBUG_INFO_TESTCASE = True, as it definitely does not depend on debug info.

abhishek.aggarwal marked 2 inline comments as done.May 26 2017, 2:19 AM

Thanks for your suggestions. I have made changes according to feedback and submitting it here.

abhishek.aggarwal edited edge metadata.

Updating D33434: Added new API to SBStructuredData class

  • Removed inferior from test case (not required)
  • fixed enum scope
abhishek.aggarwal edited the summary of this revision. (Show Details)May 26 2017, 2:24 AM
clayborg accepted this revision.May 26 2017, 10:01 AM
This revision is now accepted and ready to land.May 26 2017, 10:01 AM