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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D80150#2045364 <https://reviews.llvm.org/D80150#2045364>, @vsk wrote:

> @labath Agreed on all points, I've addressed the feedback in 82dbf4aca84 <https://reviews.llvm.org/rG82dbf4aca84ec889d0dc390674ff44e30441bcfd> by moving "DataFormatters/Mock.h" to "Plugins/Language/ObjC/Utilities.h", and adding a separate LanguageObjCTests unit test.


Cool. Thanks.



================
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);
----------------
vsk wrote:
> labath wrote:
> > 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` ?
> Yes, thank you! It looks like Eric caught this before I did.
Actually, thinking about that further, (for 64-bit `time_t`s), `double(numeric_limits<time_t>::max())` is [[ https://godbolt.org/z/t3iSd7 | exactly the same value ]] as `double(numeric_limits<time_t>::max())+1.0` because `double` doesn't have enough bits to represent the value precisely. So, I have a feeling these checks are still not testing the exact thing you want to test (though I'm not sure what that is exactly).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80150