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 std::chrono/date conflict for mac builds with C++20 #22138

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

skottmckay
Copy link
Contributor

Description

Fix usage of c++ std::chrono::operator<< in mac builds for wider range of xcode/targets.

Motivation and Context

#21033

@snnn
Copy link
Member

snnn commented Sep 19, 2024

We may also consider using absl::date, then we can totally get rid of the third-party date library, regardless C++ standard version.
For example:

// Construct an absl::Time from the system clock.
absl::Time t1 = absl::Now();

// When formatting a time, a time zone should be passed.
absl::TimeZone utc =  absl::UTCTimeZone();
std::cout << absl::FormatTime(t1, utc);

@skottmckay
Copy link
Contributor Author

What's the difference between absl::date and the date library from Howard Hinnant that we use? Both are third-party aren't they?

I would have assumed the latter is closer to what is in the C++20 standard given Howard Hinnant is on the C++ standards committee.

@snnn
Copy link
Member

snnn commented Sep 19, 2024

Howard Hinnant is out of maintenance. On the other hand, absl::date is not deprecated, and it is already in the list of ORT's dependencies. Anyway we only use it in very a few places. The change would be local.

// C++20 has operator<< in std::chrono for Timestamp type but mac builds need additional checks
// to ensure usage is valid.
// TODO: As we enable C++20 on other platforms we may need similar checks.
#define _USE_CXX20_STD_CHRONO __cplusplus >= 202002L
Copy link
Member

Choose a reason for hiding this comment

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

nit: the macro name looks like a compiler internal macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a temporary define which is undef'd as soon as used. Is there a better format to use?

snnn
snnn previously approved these changes Sep 19, 2024
@snnn snnn dismissed their stale review September 19, 2024 01:07

You need to run clang-format on the changed files.

@skottmckay
Copy link
Contributor Author

Howard Hinnant is out of maintenance. On the other hand, absl::date is not deprecated, and it is already in the list of ORT's dependencies. Anyway we only use it in very a few places. The change would be local.

I assume that's because it was added to the C++20 standard: https://github.com/HowardHinnant/date#standardization

Ideally we'd only use one date library. If there's other functionality from absl::date we need I'm fine with that being the choice (assuming no significant binary size impact). But if this is all short term and we expect the C++20 features to remove the need for a third part date library I'd leave things as-is for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants