Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for timestamps #70

Merged
merged 18 commits into from
Jun 5, 2024
Merged

Support for timestamps #70

merged 18 commits into from
Jun 5, 2024

Conversation

jjerphan
Copy link
Collaborator

@jjerphan jjerphan commented Apr 9, 2024 •
edited
Loading

Fix #52.

@jjerphan jjerphan marked this pull request as ready for review April 10, 2024 15:19
@jjerphan jjerphan marked this pull request as draft April 16, 2024 09:12
@jjerphan
Copy link
Collaborator Author

P0355 has not been implemented entirely by libc++ which has to be used by clang on conda-forge for ABI compatibility and for runtime uniqueness.

The plan is vendor parts of HowardHinnant/date and use it when -stdlib=libc++.

@jjerphan jjerphan force-pushed the timestamps branch 6 times, most recently from 96f0ef8 to 2c321b6 Compare April 22, 2024 16:11
@jjerphan jjerphan marked this pull request as ready for review April 22, 2024 16:11
@jjerphan
Support for timestamps
f85ba70 
Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan @Klaim
Test both macOS 13 and macOS 14
423082e 
Signed-off-by: Julien Jerphanion <[email protected]>

Co-authored-by: Klaim <[email protected]>
JohanMabille
JohanMabille approved these changes Apr 25, 2024
View reviewed changes
Copy link
Collaborator

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need additional tests to check compilation since all_base_types_t is already instantiated in the metaprogramming tests.

environment-dev.yml Outdated Show resolved Hide resolved
@jjerphan jjerphan force-pushed the timestamps branch 2 times, most recently from 8f358a5 to 05adbcc Compare April 25, 2024 15:10
@jjerphan
Copy link
Collaborator Author

For reference, feature testing macros aren't present in stdlibc++ 12 and 13. I have not checked for stdlibc++14 yet, but we can't expect to use it, I believe.

jjerphan
jjerphan commented May 16, 2024
View reviewed changes
include/sparrow/data_type.hpp Outdated
#include <version>
#include <chrono>

#if __cpp_lib_chrono > 201907L
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__cpp_lib_chrono (like any other FTM) might not be implemented in compiler, and it thus unusable.

See the discussions in llvm/llvm-project#73953.

@jjerphan
Copy link
Collaborator Author

As discussed, let's use HowardHinnant/date everywhere until all the implementations of the C++20 standard libraries are available.

@SylvainCorlay SylvainCorlay force-pushed the timestamps branch 18 times, most recently from c1d5493 to 1d515c3 Compare June 4, 2024 08:41
@SylvainCorlay
Copy link
Collaborator

I iterated on the PR

  • added a cmake flag -D USE_DATE_POLYFILL defaulting to ON, determining whether to use howardhinnant_date or std::chrono.
  • added tests to the matrix for testing with both howardhinnant_date and std::chrono on platforms where it is supported.

I removed the mingw tests because

  • there is no MinGW build of howardhinnant_date.
  • the GCC version used by MinGW is 12, which does not support the required features yet. We will be able to re-add this when the windows runners include GCC 13.

On a separate note, a difference between the polyfill and std::chrono is that the literals in the polyfill are prefixed with _ so I removed the use of the literals in the tests.

jjerphan
jjerphan commented Jun 4, 2024
View reviewed changes
Copy link
Collaborator Author

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Sylvain. I just have one suggestion before merging this PR.

For the readers' information, those issues are worth reading and watching:

.github/workflows/windows.yml Outdated
- {compiler: default}
- {compiler: msvc}
- {compiler: clang}
- {compiler: msvc, date-polyfill: 'ON' }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I am subscribed to GitHub Action Runners' new releases.

Suggested change
- {compiler: msvc, date-polyfill: 'ON' }
# TODO: Reintroduce default (or just mingw) when the new version is release
# See: https://github.com/actions/runner-images/commits/main/images/windows/Windows2022-Readme.md
# See: https://www.mingw-w64.org/changelog/
- {compiler: msvc, date-polyfill: 'ON' }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we explicitely invoke mingw rather than calling it because it is the default toolchain with Ninja.

Alex-PLACET
Alex-PLACET reviewed Jun 5, 2024
View reviewed changes
test/test_typed_array_timestamp.cpp Outdated Show resolved Hide resolved
test/test_typed_array_timestamp.cpp Outdated Show resolved Hide resolved
@jjerphan
Remove duplicated guard
a85ecaa 
Co-authored-by: Alexis Placet <[email protected]>

Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan
Define a constant for Unix Time
a554e0f 
Co-authored-by: Alexis Placet <[email protected]>

Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan
Use LLVM/Clang 17
6a37966 
This version or a newer on is expected by the Visual Studio2022.

See: https://github.com/xtensor-stack/sparrow/actions/runs/9381890699/job/25832125441?pr=70

Signed-off-by: Julien Jerphanion <[email protected]>
@JohanMabille JohanMabille merged commit 3fa5e86 into man-group:main Jun 5, 2024
31 checks passed
@jjerphan jjerphan deleted the timestamps branch June 5, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SylvainCorlay SylvainCorlay SylvainCorlay left review comments

@Klaim Klaim Klaim left review comments

@JohanMabille JohanMabille JohanMabille approved these changes

@Alex-PLACET Alex-PLACET Alex-PLACET approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support timestamps
5 participants
@jjerphan @SylvainCorlay @Klaim @Alex-PLACET @JohanMabille