[Lldb-commits] [PATCH] D112180: Libcpp bitset syntethic children and tests

[Raphael Isemann via Phabricator via lldb-commits]

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