[libcxx-commits] [PATCH] D56692: More calendar bits for <chrono>

[Marshall Clow via Phabricator via libcxx-commits]

mclow.lists marked 4 inline comments as done.
mclow.lists added inline comments.


================
Comment at: include/chrono:2734
 
+struct __undocumented { constexpr explicit __undocumented() noexcept = default; };
+
----------------
EricWF wrote:
> I don't think the name `__undocumented` describes what it is this does. It's used as a unspellable tag used to make constructors uncallable by the user.  Maybe `__private_ctor_t` is a better name?
> 
> That said, I think I would prefer making the constructors private instead. Especially for types like `link` which should only be created in one or two places.
My idea is that this will not be the only place we use `__undocumented` - and not just in constructors.


================
Comment at: include/chrono:2748
+private:
+    string __name;
+    string __target;
----------------
EricWF wrote:
> I think there is a better representation for this.
> 
> I have a couple of ideas, but it depends on what the implementation for `time_zone` and `tzdb` look like.
> For example, the `target()` string should always correspond to a `time_zone` with the same name. We should be able to re-use that string instead of allocating another.
> 
> And if we can't re-use strings that we already store, I was thinking something like this:
> ```
> struct Link2 {
>     Link2(std::string_view xname, std::string_view xtarget) : name_len(xname.size()), target_len(xtarget.size()) {
>         buff.reset(new char[name_len + target_len + 2] );
>         memcpy(buff.get(), xname.data(), name_len + 1);
>         memcpy(buff.get() + xname.size() + 1, xtarget.data(), target_len + 1);
>     }
>     
>     std::string_view name() { return std::string_view(buff.get(), name_len); }
>     std::string_view target() { return std::string_view(buff.get() + name_len + 1, target_len); }
>     std::unique_ptr<char[]> buff;
>     unsigned short name_len, target_len;
> };
> ```
> 
> This gives us one allocation and a object size of 16 bytes (as opposed to 64), a less expensive move operation, and more consistent invalidation semantics on the `string_view`.
You know what I like better? store a `string` and a `string_view` and construct from a `string` + `time_zone`. Note that an overwhelming majority of the names will fit in the short string optimization.


================
Comment at: include/chrono:2752
+
+inline constexpr bool operator==(const link& __lhs, const link& __rhs) noexcept {return __lhs.name() == __rhs.name();}
+inline constexpr bool operator< (const link& __lhs, const link& __rhs) noexcept {return __lhs.name()  < __rhs.name();}
----------------
EricWF wrote:
> These shouldn't be `constexpr`. 
Right. Copy/paste.


================
Comment at: include/chrono:2765
+
+    inline explicit constexpr leap (const sys_seconds & __d, __undocumented) : __date{__d} /*?? noexcept*/ {}
+
----------------
EricWF wrote:
> EricWF wrote:
> > The `inline` is redundant.
> This constructor shouldn't throw, so we can probably marke it as `noexcept`.
You're right.


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

https://reviews.llvm.org/D56692