teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
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.
================
Comment at: lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp:20
-class BitsetFrontEnd : public SyntheticChildrenFrontEnd {
+// This class can be used for handling bitsets from both libcxx and libstdcpp.
+class GenericBitsetFrontEnd : public SyntheticChildrenFrontEnd {
----------------
nit: `///` to make this into a proper doc string.
================
Comment at: lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp:23
public:
- BitsetFrontEnd(ValueObject &valobj);
+ enum class StdLib {
+ LibCxx,
----------------
I like it!
================
Comment at: lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp:90
+ // template argument.
+ ValueObjectSP dereferenced_type = m_backend.GetSP();
+ if (m_backend.GetCompilerType().IsPointerOrReferenceType()) {
----------------
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).
================
Comment at:
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py:66
+ self.check("ptr", 13)
+ self.expect("p ref",
+ substrs=['[0] = false',
----------------
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).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112180/new/
https://reviews.llvm.org/D112180
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
