This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by danilashtefan on Oct 20 2021, 1:55 PM.

Details

Summary

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

Diff Detail

Event Timeline

danilashtefan created this revision.Oct 20 2021, 1:55 PM
danilashtefan requested review of this revision.Oct 20 2021, 1:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2021, 1:55 PM
teemperor requested changes to this revision.Oct 21 2021, 5:25 AM
teemperor added a subscriber: teemperor.

Thanks for working on this!

Some notes/nits:

  • nit: The patch seems to have a bunch of unrelated clang-format changes for CPlusPlusLanguage.cpp in it. You can prevent that by using git clang-format on your local git commit before putting the diff on Phabricator (git tells clang-format which lines were modified by you and clang-format will only format those).
  • LibStdcppBitset.cpp seems to be a 99% identical copy of LibCxxBitset.cpp. I think those two classes can just be one class with some variable for the different field name in each implementation.
  • Same for the test. I think we can merge that into something like .../data-formatter-stl/generic/bitset and then just have two test_... methods where one is decorated for libc++ and one for libstdc++. You can change the Makefile values for each test with the dictionary argument of self.build (something like self.build(dictionary={"USE_LIBSTDCPP":"1"}) should work I believe).
lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
9

I think the idea is to keep this sorted alphabetically (even though this position probably make more sense, but oh well...)

This revision now requires changes to proceed.Oct 21 2021, 5:25 AM
wallace commandeered this revision.Oct 22 2021, 2:26 PM
wallace edited reviewers, added: danilashtefan; removed: wallace.
wallace updated this revision to Diff 381653.Oct 22 2021, 2:27 PM

addressed all comments.
I was able to dedup all the implementation files and the tests, and indeed passing
the make flags to the build method was a good trick.

danilashtefan commandeered this revision.Oct 23 2021, 1:47 AM
danilashtefan edited reviewers, added: wallace; removed: danilashtefan.
teemperor requested changes to this revision.Oct 23 2021, 3:49 AM

Thanks! This is looking pretty good, I just have some final minor comments about some code that I think we can also drop/split-out.

lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp
20–21

nit: /// to make this into a proper doc string.

23–28

I like it!

93

If you look in CPlusPlusLanguage.cpp there is a thing called stl_deref_flags and you can just use the same two lines that define that variable in the Libstdc++ init function. The stl_deref_flags.SetFrontEndWantsDereference(); will set a flag that does this whole deref-stuff here automatically (that's why this code wasn't necessary for the libc++ provider).

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py
66

is there a specific reason for these added self.expect("p ...", substrs=[<many lines>]) checks? I think testing the formatter with the expression evaluator is good, but there are far less verbose ways to test this (to be specific, self.primes could be a list of ValueCheck objects and you can pass that same ValueObject list as the result_children/children parameters to self.expect_expr or self.expect_var_path. This could be done in check so it would also cover all the checks in this test.).

So I would suggest we drop the added checks here and instead leave this up to a separate commit that refactors the test (you can do that if you want, but I don't mind doing it myself). This commit is in the current state just repurposing the existing libcxx formatter (with all the potentially existing bugs), so I think this is good enough for landing without expanding the test. Otherwise I think these checks could be implemented in a better way with the ValueCheck approach (see above).

This revision now requires changes to proceed.Oct 23 2021, 3:49 AM

I have corrected everything according to the previous comments. Wallace will kindly help me to format the code, since I have the following formatter error Stack dump:
0. Program arguments: clang-format -lines=1:149 -lines=0:0 lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp
/lib/x86_64-linux-gnu/libLLVM-10.so.1(_ZN4llvm3sys15PrintStackTraceERNS_11raw_ostreamE+0x1f)[0x7fa7b5d204ff]
/lib/x86_64-linux-gnu/libLLVM-10.so.1(_ZN4llvm3sys17RunSignalHandlersEv+0x50)[0x7fa7b5d1e7b0]
/lib/x86_64-linux-gnu/libLLVM-10.so.1(+0x981ac5)[0x7fa7b5d20ac5]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x153c0)[0x7fa7bc4f03c0]
/lib/x86_64-linux-gnu/libclang-cpp.so.10(_ZNK5clang13SourceManager16translateLineColENS_6FileIDEjj+0xcf)[0x7fa7ba1cc83f]
clang-format[0x407e10]
clang-format[0x406b64]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7fa7b4e4e0b3]
clang-format[0x40652e]
error: clang-format -lines=1:149 -lines=0:0 lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp failed

wallace requested changes to this revision.Oct 25 2021, 2:31 PM

almost there!

lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp
20–21

Use /// instead of //, that way it'll appear in the official documentation

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py
32–35

What teemperor is saying is to something like:

children = [ValueCheck(name="<something goes here>", summary='<same here>')]
for i in range(size):
  children.append(
valobj = self.expect_var_path(
          name,
          type="std::bitset<" + size + ">", # you might need to change this depending on the stdlib you use for each test
          children=children,
      )

self.assertRegex(valobj.summary, r'^"hello"( strong=1)? weak=1$') # change this for the actual summary of the object
This revision now requires changes to proceed.Oct 25 2021, 2:31 PM
wallace added inline comments.Oct 25 2021, 2:50 PM
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py
32–35

Ugh, I made some mistakes

children = []
for i in range(size):
  children.append(
valobj = self.expect_var_path( // this will check the entire bitset and the children argument will check its children
          name,
          type="std::bitset<" + size + ">", // you might need to change this depending on the stdlib you use for each test
          children=children,
      )

self.assertRegex(valobj.summary, r'^"hello"( strong=1)? weak=1$') # change this for the actual summary of the object

Minor corrections

wallace retitled this revision from Libcpp bitset syntethic children and tests to [formatters] Add a libstdcpp formatter for bitset and unify tests across stdlibs.Oct 26 2021, 9:04 AM
wallace edited the summary of this revision. (Show Details)
wallace retitled this revision from [formatters] Add a libstdcpp formatter for bitset and unify tests across stdlibs to [formatters] Add a libstdcpp formatter for set and unify tests across stdlibs.
wallace edited the summary of this revision. (Show Details)
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

With this changes I substitute the verbose long printed check with ValueCheck approach

wallace retitled this revision from [formatters] Add a libstdcpp formatter for set and unify tests across stdlibs to [formatters] Add a libstdcpp formatter for bitset and unify tests across stdlibs.Oct 26 2021, 1:52 PM
wallace edited the summary of this revision. (Show Details)
danilashtefan updated this revision to Diff 382463.EditedOct 26 2021, 2:21 PM

Refactoring

wallace accepted this revision.Oct 26 2021, 2:23 PM

great! I'll land it for you

This revision was not accepted when it landed; it landed in state Needs Review.Oct 26 2021, 2:49 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.