[k2] add Date/Time classes to light runtime #1481
Conversation
Shamzik
force-pushed
the
kshamazov/datetime
branch
from
d1df9ec to
244c045
Compare
Shamzik
changed the title
| class string { | ||
| public: | ||
| using size_type = string_size_type; | ||
| using value_type = char; |
There was a problem hiding this comment.
What's the purpose of this change?
There was a problem hiding this comment.
This was added to be able to use std::back_inserter
Shamzik
force-pushed
the
kshamazov/datetime
branch
from
49fc0f4 to
a96050c
Compare
apolyakov
left a comment
apolyakov
left a comment
There was a problem hiding this comment.
I've reviewed and noted some comments. Please, take into account that these comments should be applied to whole PR, not only the places where I left those comments
| inline constexpr std::string_view BACKSLASH_ = "\\"; | ||
| inline constexpr std::string_view QUOTE_ = "\""; | ||
| inline constexpr std::string_view NEWLINE_ = "\n"; | ||
| inline constexpr std::string_view NOW_ = "now"; |
There was a problem hiding this comment.
It's the context for strings part of standard library. Let's move this to one specific for time functions
|
|
||
| #include "kphp/timelib/timelib.h" | ||
|
|
||
| #include "common/containers/final_action.h" |
There was a problem hiding this comment.
Please, remove all the unused includes
|
|
||
| using error_container_t = std::unique_ptr<timelib_error_container, destructor>; | ||
| using rel_time_t = std::unique_ptr<timelib_rel_time, destructor>; | ||
| using time_t = std::unique_ptr<timelib_time, destructor>; |
There was a problem hiding this comment.
Now we have a mix of raw timelib types and our own wrappers in the API. It doesn't look like a good solution.
Also, please refactor exisiting code that uses raw types in case you add wrappers. Mixing types is always bad
There was a problem hiding this comment.
fixed except clone function
|
|
||
| time_offset_t construct_time_offset(timelib_time& t) noexcept; | ||
|
|
||
| std::pair<time_t, error_container_t> parse_time(std::string_view time_sv) noexcept; |
There was a problem hiding this comment.
Is it possible for us to have both valid time_t and error_container_t?
There was a problem hiding this comment.
error_container_t is always non-null here. time_t is null in case of an error.
|
|
||
| std::pair<time_t, error_container_t> parse_time(std::string_view time_sv) noexcept; | ||
| std::pair<time_t, error_container_t> parse_time(std::string_view time_sv, const char* format) noexcept; | ||
| std::expected<rel_time_t, std::format_string<const char*>> parse_interval(std::string_view format) noexcept; |
There was a problem hiding this comment.
Hmm, I think it's not the best idea to return error format string instead of an error itself
|
|
||
| time_t add(timelib_time& t, timelib_rel_time& interval) noexcept; | ||
|
|
||
| rel_time_t clone(timelib_rel_time& rt) noexcept; |
There was a problem hiding this comment.
Isn't it better to just create a wrapper class? As far as I see, it might be worth it since you already have clone and destruct
There was a problem hiding this comment.
I've refactored the header to make it easier to read. This library isn't very friendly to creating wrapper classes around it.
|
|
||
| void set_timestamp(timelib_time& t, int64_t timestamp) noexcept; | ||
|
|
||
| std::string_view english_suffix(timelib_sll number) noexcept; |
There was a problem hiding this comment.
Should it really be a part of public kphp::timelib API?
There was a problem hiding this comment.
hided in details namespace
This first part of implementing Date/Time classes. Here implemented: