This is an archive of the discontinued LLVM Phabricator instance.

[formatters] Add a libstdcpp formatter for set and unify tests across stdlibs
ClosedPublic

Authored by danilashtefan on Oct 26 2021, 7:27 AM.

Details

Summary

This diff adds a data formatter for libstdcpp's set. Besides, it unifies the tests for set for libcxx and libstdcpp for maintainability.

Diff Detail

Event Timeline

danilashtefan created this revision.Oct 26 2021, 7:27 AM
danilashtefan requested review of this revision.Oct 26 2021, 7:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2021, 7:27 AM

Set synthetic children

Set synthetic child

danilashtefan retitled this revision from Set synthetic children. I have dove the generic tests for set. I have considered Libcxx tests as the one covering all the edge cases (including pointer and reference values check). So to make them pass I had to change some stl_summarry_flags... to Set synthetic children.Oct 26 2021, 8:16 AM
danilashtefan edited the summary of this revision. (Show Details)
danilashtefan added a reviewer: wallace.

Deleted libcxx tests

wallace requested changes to this revision.Oct 26 2021, 9:11 AM

I think you messed up when updating the diffs. I don't see the bitset diff anymore.

This revision now requires changes to proceed.Oct 26 2021, 9:11 AM
wallace retitled this revision from Set synthetic children to [formatters] Add a libstdcpp formatter for set and unify tests across stdlibs.Oct 26 2021, 2:06 PM
wallace edited the summary of this revision. (Show Details)
wallace added a reviewer: clayborg.

just some minor things left

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
941–942

to mimic the libcxx case that you can see in the line 710 of https://lldb.llvm.org/cpp_reference/CPlusPlusLanguage_8cpp_source.html, you can do stl_summary_flags.SetSkipPointers(false) in line 932 of this file. Then both standard libraries will be handled the same way. Make sure to execute the relevant tests for vector, map, set and list for libstdcpp.
This also means that you don't need the new variable stl_set_summary_flags

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/set/Makefile
4

remove this line

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/set/TestDataFormatterGenericSet.py
45

can you add additional assertions like the one you did in https://reviews.llvm.org/D112180 that uses ValueCheck?

Besides, given that you are using the same python implementation for map and set, better rename the class to StdSeOrMapSynthProvider and add a comment mentioning that it works both for set and maps because they have the same underlying structure

danilashtefan updated this revision to Diff 382665.EditedOct 27 2021, 7:17 AM
danilashtefan edited the summary of this revision. (Show Details)

Thank you for the comments, I have done everything specified!

danilashtefan added inline comments.Oct 27 2021, 7:24 AM
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/set/TestDataFormatterGenericSet.py
45

I have made check method more generic and less verbose. Before it was specifically checking the "ii" variable, that was a set<int> of size 7. With value check approach we can now test set<string> and others.

I guess, that I can delete all of the

self.expect()

and simply call check method with the expected size. Please, let me know if I should do it or leave it as it is. Thanks

Small formatting

wallace requested changes to this revision.Oct 27 2021, 9:09 AM

pretty good! Just some cosmetic changes needed and that's it

lldb/examples/synthetic/gnu_libstdcpp.py
318 ↗(On Diff #382668)

Make a comment above like

'''
Set and Map have the same underlying data structure, therefore we can use exactly the same implementation for the formatter.
'''
324–325 ↗(On Diff #382668)

As this is not just for maps, you can do something like:

self.kind = "set" if "set" in valobj.GetTypeName() else "map"
logger >> "Providing synthetic children for a " self.kind + " named " + \
          str(valobj.GetName())
352 ↗(On Diff #382668)
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/set/TestDataFormatterGenericSet.py
153–154

remove these lines

This revision now requires changes to proceed.Oct 27 2021, 9:09 AM
wallace added inline comments.Oct 27 2021, 9:46 AM
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/set/TestDataFormatterGenericSet.py
45

yes, the code is good as you did it

wallace accepted this revision.Oct 27 2021, 11:22 AM

Just fix that last thing and I'll land your patch :) Thanks

lldb/examples/synthetic/gnu_libstdcpp.py
318 ↗(On Diff #382668)

actual python documentation uses ''' for comments, not #

This revision is now accepted and ready to land.Oct 27 2021, 11:22 AM

Last thing corrected :)

This revision was landed with ongoing or failed builds.Oct 27 2021, 11:55 AM
This revision was automatically updated to reflect the committed changes.