[Lldb-commits] [PATCH] D101453: [lldb] Add tests for DumpDataExtractor formats

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

FWIW, we do have a test for this named TestBuiltinFormats.py. It's using the SB API so it's not as a direct test as this one, so I think having this too is good.

I think this looks good otherwise so let's ship it. I have a few comments about the comments though, but that can be fixed while landing.



================
Comment at: lldb/unittests/Core/DumpDataExtractorTest.cpp:27
+                          endian::InlHostByteOrder(),
+                          /*address size*/ 4);
+  DumpDataExtractor(extractor, &result, 0, format, data_size, item_count, 1, 0,
----------------
Please change that to something like `/*address_size=*/` where `address_size` is the exact argument name. This way clang-tidy can check the name for us.


================
Comment at: lldb/unittests/Core/DumpDataExtractorTest.cpp:74
+  // test_format(std::complex<long double>(1.1, 2.2), lldb::eFormatComplex,
+  //             "0x00000000: 1.1 + 2.2i");
+
----------------
I think the explanation is already good enough (and commented out code isn't great).


================
Comment at: lldb/unittests/Core/DumpDataExtractorTest.cpp:79
+  test_format<uint16_t>(99, lldb::Format::eFormatDecimal, "0x00000000: 99");
+  // Just prints as a signed integer
+  test_format(-1, lldb::Format::eFormatEnum, "0x00000000: -1");
----------------
Can you put a period behind that comment and the others? Just being nit-picky here :)


================
Comment at: lldb/unittests/Core/DumpDataExtractorTest.cpp:175
+  DataExtractor extractor(dumpbuffer.GetBytes(), dumpbuffer.GetByteSize(),
+                          endian::InlHostByteOrder(), /*address size*/ 4);
+
----------------
same as above, please `/*address_size=*/`


================
Comment at: lldb/unittests/Core/DumpDataExtractorTest.cpp:178
+  DumpDataExtractor(extractor, &result, 0, lldb::Format::eFormatCharArray,
+                    // item_byte_size
+                    1,
----------------
Same as above, `/*item_byte_size=*/1` (and the same below).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101453