[Lldb-commits] [PATCH] D80150: [lldb/DataFormatter] Check for overflow when finding NSDate epoch

[Pavel Labath via Phabricator via lldb-commits]

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