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

[Eric Fiselier via Phabricator]

EricWF requested changes to this revision.
EricWF added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/chrono:2734
 
+struct __undocumented { constexpr explicit __undocumented() noexcept = default; };
+
----------------
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.


================
Comment at: include/chrono:2739
+public:
+    link            (link&&) = default;
+    link& operator= (link&&) = default;
----------------
Move operations should be `noexcept`.


================
Comment at: include/chrono:2742
+
+    inline explicit link(string __nm, string __tg, __undocumented) : __name{__nm}, __target{__tg} {}
+
----------------
This signature smells funky.

I think it should have `string_view` parameters instead. That way we're not double copying a string.

Of course,  if `link` is only ever constructed from arguments of type `string&&`, then the parameter type should be string and the arguments should be `std::move`ed .



================
Comment at: include/chrono:2748
+private:
+    string __name;
+    string __target;
----------------
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`.


================
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();}
----------------
These shouldn't be `constexpr`. 


================
Comment at: include/chrono:2765
+
+    inline explicit constexpr leap (const sys_seconds & __d, __undocumented) : __date{__d} /*?? noexcept*/ {}
+
----------------
The `inline` is redundant.


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


================
Comment at: include/chrono:2782
+template <class _Duration>
+  constexpr bool operator==(const leap& __lhs, const sys_time<_Duration>& __rhs) noexcept { return __lhs.date() == __rhs; }
+
----------------
What's with this indentation? `clang-format` all the things!


================
Comment at: test/std/utilities/time/time.zone/time.zone.link/types.pass.cpp:30
+{
+    using link        = std::chrono::link;
+
----------------
Weird indentation. 


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

https://reviews.llvm.org/D56692