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

Thu Oct 21 05:25:43 PDT 2021

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

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).

================
Comment at: lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt:9
   LibCxxBitset.cpp
+  LibStdcppBitset.cpp
   LibCxxInitializerList.cpp
----------------
I think the idea is to keep this sorted alphabetically (even though this position probably make more sense, but oh well...)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112180/new/

https://reviews.llvm.org/D112180