This diff adds a data formatter for libstdcpp's bitset. Besides, it unifies the tests for bitset for libcxx and libstdcpp for maintainability.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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...) |
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.
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 | ||
67 | 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). |
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
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 |
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 |
With this changes I substitute the verbose long printed check with ValueCheck approach
I think the idea is to keep this sorted alphabetically (even though this position probably make more sense, but oh well...)