This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix compilation error on platforms that don't implement std::tm
ClosedPublic

Authored by ldionne on Sep 8 2022, 7:00 AM.

Details

Summary

Instead of mentioning tm directly in the definition of __convert_to_tm,
take it as a template argument. As a fly-by also fix incorrect Lit feature
(should have been no-localization instead of libcpp-has-no-localization).

Diff Detail

Event Timeline

ldionne created this revision.Sep 8 2022, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 7:00 AM
ldionne requested review of this revision.Sep 8 2022, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 7:00 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jloser added a subscriber: jloser.Sep 8 2022, 7:54 AM
jloser added inline comments.
libcxx/include/__chrono/convert_to_tm.h
14–15

Since we're taking the tm type as a template argument, we never explicitly mention std::tm now. Therefore, we should be able to remove this include, right?

ldionne marked an inline comment as done.Sep 8 2022, 7:57 AM
ldionne added inline comments.
libcxx/include/__chrono/convert_to_tm.h
14–15

Yes, that sounds right.

ldionne updated this revision to Diff 458742.Sep 8 2022, 7:57 AM
ldionne marked an inline comment as done.

Address review comments.

jloser accepted this revision.Sep 8 2022, 8:43 AM

LGTM!

Which platforms don't have tm that we support? Can we add a CI for them?

libcxx/include/__chrono/convert_to_tm.h
31

Why is this change? As mentioned during our review it looks a bit odd now. However this function will get support for all chrono types that are formattable. An example of the "final" version can be seen in the function __make_tm
in https://reviews.llvm.org/D126592#change-qGrpldbZrZTa .

libcxx/test/std/time/time.syn/formatter.day.pass.cpp
9

Thanks! Nice catch.

ldionne marked an inline comment as done.Sep 8 2022, 12:34 PM

Which platforms don't have tm that we support? Can we add a CI for them?

Apple's DriverKit is an example of that. Adding CI for that is on my todo list, but there's some stuff that needs to happen before that can be done.

libcxx/include/__chrono/convert_to_tm.h
31

Oh, yes, you're right. I'll undo that part.

ldionne updated this revision to Diff 458828.Sep 8 2022, 12:34 PM
ldionne marked an inline comment as done.

Apply review comments.

ldionne accepted this revision as: Restricted Project.Sep 8 2022, 3:10 PM
This revision is now accepted and ready to land.Sep 8 2022, 3:10 PM

Which platforms don't have tm that we support? Can we add a CI for them?

Apple's DriverKit is an example of that. Adding CI for that is on my todo list, but there's some stuff that needs to happen before that can be done.

Thanks for the info.

LGTM!

libcxx/include/__chrono/convert_to_tm.h
26

I'm not thrilled by this name, but when I have a better name I'll incorporate that in a next patch.