labath added a comment. While I think the idea in this patch is good, the actual implementation seems problematic to me, on several levels.
The name `Mock.h` is overly generic, and inappropriate. According to wikipedia <https://en.wikipedia.org/wiki/Mock_object>, mock (object)s are "simulated objects that mimic the behavior of real objects in controlled ways, most often as part of a software testing initiative". This file misses that mark completely, because it is not simulating anything -- it *is* the real thing, production code. It's true that the main reason it is even declared in a header is testing, but that does not make it a "mock". A mock "nsdate formatter" would be if we had the ability to, in a test, replace the implementation of the "convert an nsdate value to a string". Such a thing might potentially be useful as a way to test some higher level data formatter functionality, but it is definitely not what is happening here. The second issue is that the placement of the file in `lldb/DataFormatter` breaks encapsulation. The generic data formatter code should have no knowledge of a particular language (runtime) implementation details. That might be excused if this particular functionality was useful for more than one language, but I don't see any evidence that this is the case. What's worse, by putting the declaration into `lldb/DataFormatter`, but having the implementation in `Language/ObjC` you're making it very easy to unknowingly depend on a plugin. Last, but not least, it is against the llvm library layering rules <http://llvm.org/docs/CodingStandards.html#library-layering>. (The paths there are specific to the llvm subtree, but it's clear the intent is for the header/interface and the implementation to live together.) TL;DR: The fix is quite simple: move `lldb/DataFormatter/Mock.h` to `Plugins/Language/ObjC` and rename it to something less misleading (`Utilities.h` ?) And move the test to `unittests/Language/ObjC`. ================ Comment at: lldb/unittests/DataFormatter/MockTests.cpp:27 +TEST(DataFormatterMockTest, NSDate) { + EXPECT_EQ(*formatDateValue(-63114076800), "0001-12-30 00:00:00 +0000"); + ---------------- `EXPECT_EQ(formatDateValue(...), std::string(...))` would be better. That way we'll get a test failure if the function returns None (instead of an assertion failure due to "dereferencing" an empty optional. `Optional<string>` and `string` are comparable and pretty-print correctly. The only reason this does not work out-of-the-box is because the Optional's operator== does not accept implicit conversions due to it being a template. ================ Comment at: lldb/unittests/DataFormatter/MockTests.cpp:30 + // Can't convert the date_value to a time_t. + EXPECT_EQ(formatDateValue(std::numeric_limits<time_t>::max() + 1), + llvm::None); ---------------- Isn't this actually `std::numeric_limits<time_t>::min()` (and UB due to singed wraparound) ? Did you want to convert to double before doing the `+1` ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80150/new/ https://reviews.llvm.org/D80150 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
