Page MenuHomePhabricator

Improve the libstdc++ smart pointer formatters
ClosedPublic

Authored by tberghammer on Oct 18 2016, 6:48 AM.

Details

Summary

Improve the libstdc++ smart pointer formatters

  • Display the strong/weak count in the summary
  • Display the pointed object as a synthetic member
  • Create synthetic children for weak/strong count

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer retitled this revision from to Improve the libstdc++ smart pointer formatters.
tberghammer updated this object.
tberghammer added reviewers: labath, granata.enrico.
tberghammer added a subscriber: lldb-commits.

Move the code to a new cpp file

Please run Clang-format over new code.

source/Plugins/Language/CPlusPlus/LibStdcppSmartPointer.cpp
12 ↗(On Diff #75025)

STL headers should be after application's headers.

28 ↗(On Diff #75025)

No space before access specifiers. Same below.

Please run Clang-format over new code.

I run it before upload but it mush have picked up some strange config. Will run it again before submit. Thanks for noticing it.

labath edited edge metadata.Oct 19 2016, 3:27 AM

Looks good from my side. Enrico, do you want to have a look at this?

A couple tiny comments below. Also, be sure to properly reformat and reorder the headers.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libstdcpp/smart_ptr/TestDataFormatterStdSmartPtr.py
41 ↗(On Diff #75025)

Could you add a test that accesses a non-existing subobject of ssp, to make sure it does something not-completely-broken.

source/Plugins/Language/CPlusPlus/LibStdcppSmartPointer.cpp
27 ↗(On Diff #75025)

What do you think about renaming this class to something shorter (SharedPtrFrontEnd, SharedPtrSynthetic, simply FrontEnd ?). Since it's local to a cpp, I don't think we need to be so explicit, and it seems wasteful to use half of the line length for the class name.

89 ↗(On Diff #75025)

It would be useful to add a short comment explaining the -1

138 ↗(On Diff #75025)

I know you just copied it, but this seems wrong (size_t can be 64-bit). Will this work if you just return ~0.

I see you already got a bunch of feedback on specific items. The overall idea looks good to me. I'll try to delve a little deeper in the code ASAP (I was out for a couple days and have some backlog...), but should be good to go assuming you address the feedback you already got + make sure tests work!

tberghammer edited edge metadata.
tberghammer marked 5 inline comments as done.
tberghammer added inline comments.
source/Plugins/Language/CPlusPlus/LibStdcppSmartPointer.cpp
138 ↗(On Diff #75025)

Actually as far as I can tell all use case of this API assumes it is a uint32_t and compares the return value against UINT32_MAX :(
I will just leave it as it is and then we should have a separate CL what cleans up the mess around it

This revision was automatically updated to reflect the committed changes.