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

Update C++ to standard 20 for Windows #17706

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

Update C++ to standard 20 for Windows #17706

wants to merge 30 commits into from

Conversation

jchen351
Copy link
Contributor

@jchen351 jchen351 commented Sep 26, 2023

Description

Current blocker:

Motivation and Context

@snnn snnn requested review from baijumeswani, wejoncy, HectorSVC and edgchen1 and removed request for baijumeswani and wejoncy June 4, 2024 00:03
@@ -55,7 +55,7 @@ class QnnBackendManager {
~QnnBackendManager();
char* DlError() {
#ifdef _WIN32
return "";
return nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest splitting this change out to a separated PR and ask the QNN EP team to review. Your this change probably won't work.

Copy link
Contributor

@edgchen1 edgchen1 Jun 4, 2024

Choose a reason for hiding this comment

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

is this related to returning a non-const char pointer to a string literal? can we just change the signature to const char* DlError()? looks like the existing usage of QnnBackendManager::DlError() should accomodate that.

@@ -65,7 +65,11 @@ void OStreamSink::SendImpl(const Timestamp& timestamp, const std::string& logger
#ifdef _WIN32
void WOStreamSink::SendImpl(const Timestamp& timestamp, const std::string& logger_id, const Capture& message) {
// operator for formatting of timestamp in ISO8601 format including microseconds
using date::operator<<;
#if __cplusplus >= 202002L
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use timestamp_ns::operator<< here to reduce the ifdefs.

the ifdefs here will need to be updated to work for Windows:

// TODO: When other compilers support std::chrono::operator<<, update this.
// TODO: Check support for other compilers' version before enable C++20 for other compilers.
// Xcode added support for C++20's std::chrono::operator<< in SDK version 14.4.
#if __cplusplus >= 202002L && __MAC_OS_X_VERSION_MAX_ALLOWED >= 140400L
namespace timestamp_ns = std::chrono;
#else
namespace timestamp_ns = ::date;
#endif

@@ -193,6 +193,9 @@ void SetupUpsampleFilterAntiAlias(FilterParamsAntiAlias<T>& p,
}

float total_weight_inv = total_weight == 0.0f ? 1.f : 1.0f / total_weight;
#ifdef _WIN32
#pragma warning(disable : 4189)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is it warning about here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused variable.

@jchen351
Copy link
Contributor Author

depends on #21071

…n_c++20

# Conflicts:
#	onnxruntime/contrib_ops/cuda/math/cufft_plan_cache.h
#	orttraining/orttraining/training_ops/cuda/nn/conv_shared.h
#	orttraining/orttraining/training_ops/rocm/nn/conv_grad.cc
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.

None yet

3 participants