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.

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

Fix libc++ incompatibility #1463

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Fix libc++ incompatibility #1463

merged 1 commit into from
Nov 15, 2023

Conversation

upsj
Copy link
Member

@upsj upsj commented Nov 15, 2023

This is an issue detected in conan-io/conan-center-index#21058, which uses a different integer type for std::chrono::nanosecond

@upsj upsj added the 1:ST:ready-for-review This PR is ready for review label Nov 15, 2023
@upsj upsj requested a review from a team November 15, 2023 00:51
@upsj upsj self-assigned this Nov 15, 2023
@ginkgo-bot ginkgo-bot added the mod:core This is related to the core module. label Nov 15, 2023
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

LGTM. but maybe need to check .count() type with static_cast

Comment on lines -99 to 100
return std::chrono::duration_cast<std::chrono::nanoseconds, int64>(
return std::chrono::duration_cast<std::chrono::nanoseconds>(
stop.data_.chrono - start.data_.chrono);
Copy link
Member

Choose a reason for hiding this comment

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

Just to ensure I got it correctly.
duration_cast signature:

template< class ToDuration, class Rep, class Period >
constexpr ToDuration duration_cast( const std::chrono::duration<Rep,Period>& d );

The issue is from type definition of std::chrono::nanoseconds
std::chrono::duration</* int64 */, std::nano> standard only require it at least 64 bit signed integer.
Thus, it might be not int64.
The type also affect .count() return type.
Could you check that the .count() has static_cast to desired type if it is the case?

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is basically that I used to return int64 before and now return nanoseconds, and I didn't change the function call. It only worked accidentally.

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked, we only use .count() in places relevant to this issue where it will be cast to double, so no problems expected

@upsj upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Nov 15, 2023
@upsj upsj merged commit 6e3ca45 into develop Nov 15, 2023
13 of 16 checks passed
@upsj upsj deleted the libcxx_compat branch November 15, 2023 20:01
Copy link

sonarcloud bot commented Nov 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

warning The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants