Details
- Reviewers
EricWF - Group Reviewers
Restricted Project - Commits
- rGa4814bdc5371: [libc++][chrono] Fixes formatter duration.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/test/std/time/time.syn/formatter.duration.pass.cpp | ||
---|---|---|
1095 | Are you sure this number shouldn't be 65'535 (aka 0FFFF)? Wouldn't it be better to write them all in hex, so it's clear what they're represententing? |
libcxx/test/std/time/time.syn/formatter.duration.pass.cpp | ||
---|---|---|
1095 | Good catch, I've applied your suggestion. |
libcxx/include/__chrono/convert_to_tm.h | ||
---|---|---|
95 | nit: tabs |
libcxx/include/__chrono/convert_to_tm.h | ||
---|---|---|
88 | In what case can it be converted to seconds but not hours? Also is there a test case for the case for the case where the type cannot be converted to hours? | |
libcxx/test/std/time/time.syn/formatter.duration.pass.cpp | ||
1089 | Can't we just use std::chrono::years::max? Do these test cases fail before your fixes? It looks to me that they're actively avoiding constructing test values that will overflow. |
libcxx/include/__chrono/convert_to_tm.h | ||
---|---|---|
88 | Is the comment unclear? There are tests, I'll comment below. | |
libcxx/test/std/time/time.syn/formatter.duration.pass.cpp | ||
43 | This value can't be converted to hours. | |
1089 | These tests test the Standard mandated limits as stated by the comment on line 1081. Using larger values will make the tests non-portable. |
This is a better state of the world for sure. I still think there may be ways to do better, but perfect is the enemy of progress, so let's progress for now and think about perfect later.
libcxx/test/std/time/time.syn/formatter.duration.pass.cpp | ||
---|---|---|
1089 | Right, but it's important we don't introduce UB if a user violates the limits (which they will). |
libcxx/include/__chrono/convert_to_tm.h | ||
---|---|---|
88 | If it can't be converted to hours because it potentially overflows, why is it that converting to seconds works? | |
libcxx/test/std/time/time.syn/formatter.duration.pass.cpp | ||
1089 | I don't understand where in the standard you're finding this requirement. Could you please provide the stable name? |
Thanks for the review! I will give this approach some more thought. But I too prefer to get this fix in.
libcxx/include/__chrono/convert_to_tm.h | ||
---|---|---|
88 | Since our implementation uses the same number of bits for hours and seconds. | |
libcxx/test/std/time/time.syn/formatter.duration.pass.cpp | ||
1089 | It's in [time.syn] http://eel.is/c++draft/time.syn |
In what case can it be converted to seconds but not hours?
Also is there a test case for the case for the case where the type cannot be converted to hours?