Page MenuHomePhabricator

More calendar bits for <chrono>
Needs ReviewPublic

Authored by mclow.lists on Jan 14 2019, 5:25 PM.

Details

Reviewers
EricWF
ldionne
Group Reviewers
Restricted Project
Summary

This patch adds the types leap and link to std::chrono.
These are "building block" types, that will be used when we get all the time zone stuff in.

Note the new type __undocumented, that denotes an undocumented call.
chrono is probably not the right place for this in the long run, I expect that it will find other uses.

Diff Detail

Event Timeline

mclow.lists created this revision.Jan 14 2019, 5:25 PM
mclow.lists marked an inline comment as done.Jan 14 2019, 5:26 PM
mclow.lists added inline comments.
test/std/utilities/time/time.zone/time.zone.leap/types.pass.cpp
30 ↗(On Diff #181680)

Tabs?

EricWF requested changes to this revision.Jan 18 2019, 5:56 PM
EricWF added inline comments.
include/chrono
2734 ↗(On Diff #181680)

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.

2739 ↗(On Diff #181680)

Move operations should be noexcept.

2742 ↗(On Diff #181680)

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::moveed .

2748 ↗(On Diff #181680)

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.

2752 ↗(On Diff #181680)

These shouldn't be constexpr.

2765 ↗(On Diff #181680)

The inline is redundant.

2765 ↗(On Diff #181680)

This constructor shouldn't throw, so we can probably marke it as noexcept.

2782 ↗(On Diff #181680)

What's with this indentation? clang-format all the things!

test/std/utilities/time/time.zone/time.zone.link/types.pass.cpp
30 ↗(On Diff #181680)

Weird indentation.

This revision now requires changes to proceed.Jan 18 2019, 5:56 PM
mclow.lists marked 4 inline comments as done.Jan 30 2019, 1:05 PM
mclow.lists added inline comments.
include/chrono
2734 ↗(On Diff #181680)

My idea is that this will not be the only place we use __undocumented - and not just in constructors.

2748 ↗(On Diff #181680)

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.

2752 ↗(On Diff #181680)

Right. Copy/paste.

2765 ↗(On Diff #181680)

You're right.

mclow.lists marked 5 inline comments as done.

Update based on Eric's comments.

include/chrono
2748 ↗(On Diff #181680)

and more consistent invalidation semantics on the string_view.

The string_view should never be invalidated. Once the vector of links is created in the tzdb, then it never moves/changes/gets deallocated. Before the tzdb is created, no one calls name or target.

As for the allocations, as I said before, "an overwhelming majority of the names will fit in the short string optimization." (~560 out of ~590 across both zones and links). So most of the time, using strings involves NO allocations.

ldionne added a reviewer: Restricted Project.Nov 2 2020, 3:00 PM

Marshall, are you ready to commit this? I haven't looked deeply, but it looks like you had addressed Eric's comments?

libcxx/test/std/utilities/time/time.zone/time.zone.link/types.pass.cpp
35

Weird indentation.

I'm fine with rebasing and committing this.

ldionne set the repository for this revision to rG LLVM Github Monorepo.Mon, Jan 11, 3:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jan 11, 3:01 PM

Shouldn't this patch implement also the renames of leap and link?
https://wg21.link/P1981 Rename leap to leap_second
https://wg21.link/P1982 Rename link to time_zone_link

Shouldn't this patch implement also the renames of leap and link?

Yes. And re-do the comparisons to use spaceship.