-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Fix Clang + libc++ compilation and portability issues #28049
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| // Portable math constant definitions. | ||
| // POSIX constants like M_PI and M_SQRT2 are not part of the C++ standard and | ||
| // are not provided by all standard library implementations (e.g. libc++). | ||
| // This header provides them when missing, enabling compilation with any | ||
| // conforming C++ standard library. | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <cmath> | ||
|
|
||
| #ifndef M_PI | ||
| #define M_PI 3.14159265358979323846 | ||
| #endif | ||
|
|
||
| #ifndef M_SQRT1_2 | ||
| #define M_SQRT1_2 0.70710678118654752440 | ||
| #endif | ||
|
|
||
| #ifndef M_SQRT2 | ||
| #define M_SQRT2 1.41421356237309504880 | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,7 +106,7 @@ CumSum<T>::CumSum(const OpKernelInfo& info) : OpKernel(info), exclusive_(), reve | |
| if (exclusive == 1 || exclusive == 0) { | ||
| exclusive_ = exclusive; | ||
| } else { | ||
| ORT_ENFORCE("attribute exclusive can only be 0 or 1"); | ||
| ORT_ENFORCE(false, "attribute exclusive can only be 0 or 1"); | ||
| } | ||
| } | ||
| int64_t reverse = 0; | ||
|
Comment on lines
106
to
112
|
||
|
|
@@ -115,7 +115,7 @@ CumSum<T>::CumSum(const OpKernelInfo& info) : OpKernel(info), exclusive_(), reve | |
| if (reverse == 1 || reverse == 0) { | ||
| reverse_ = reverse; | ||
| } else { | ||
| ORT_ENFORCE("attribute reverse can only be 0 or 1"); | ||
| ORT_ENFORCE(false, "attribute reverse can only be 0 or 1"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |||
| #include "core/mlas/inc/mlas.h" | ||||
| #include "core/platform/threadpool.h" | ||||
| #include "core/common/inlined_containers.h" | ||||
| #include "core/common/math_constants.h" | ||||
|
||||
| #include "core/common/math_constants.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Now uses M_PI and M_SQRT2 from math_constants.h.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description mentions providing additional constants like
M_LOG2E, etc., but this new header currently only definesM_PI,M_SQRT1_2, andM_SQRT2. Either extend this header to include the other promised constants (as needed by the build) or update the PR description to match what’s actually being introduced.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated PR description to match the constants actually defined.