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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits