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).
Details
- Reviewers
jloser - Group Reviewers
Restricted Project - Commits
- rGd529e8110bdf: [libc++] Fix compilation error on platforms that don't implement std::tm
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/include/__chrono/convert_to_tm.h | ||
|---|---|---|
| 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? | |
| libcxx/include/__chrono/convert_to_tm.h | ||
|---|---|---|
| 15 | Yes, that sounds right. | |
Which platforms don't have tm that we support? Can we add a CI for them?
| libcxx/include/__chrono/convert_to_tm.h | ||
|---|---|---|
| 32 | 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 | |
| libcxx/test/std/time/time.syn/formatter.day.pass.cpp | ||
| 9 | Thanks! Nice catch. | |
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 | ||
|---|---|---|
| 32 | Oh, yes, you're right. I'll undo that part. | |
Thanks for the info.
LGTM!
| libcxx/include/__chrono/convert_to_tm.h | ||
|---|---|---|
| 27 | I'm not thrilled by this name, but when I have a better name I'll incorporate that in a next patch. | |
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?