Page MenuHomePhabricator

More calendar bits for <chrono>
Needs ReviewPublic

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


Group Reviewers
Restricted Project

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.
30 ↗(On Diff #181680)


EricWF requested changes to this revision.Jan 18 2019, 5:56 PM
EricWF added inline comments.
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(),, name_len + 1);
        memcpy(buff.get() + xname.size() + 1,, 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!

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

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?


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? Rename leap to leap_second 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.