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

Deprecate Math<T> in favor of std:: #65

Merged
merged 6 commits into from
Dec 5, 2020

Conversation

lgritz
Copy link
Contributor

@lgritz lgritz commented Nov 15, 2020

These were helpful to allow templated application of either double or
float in the olden days, but in C++11 and beyond, std:: versions of
these math functions do the same thing and should be preferred.

For back compatibility for applications, I'm keeping Math but
defining them to just be the std:: versions. But the goal should be
to remove them eventually.

Signed-off-by: Larry Gritz lg@larrygritz.com

These were helpful to allow templated application of either double or
float in the olden days, but in C++11 and beyond, std:: versions of
these math functions do the same thing and should be preferred.

For back compatibility for applications, I'm keeping Math<T> but
defining them to just be the std:: versions. But the goal should be
to remove them eventually.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

This is great. I have a strong feeling we don't need to keep the Math::foo() functions; my argument as in lots of other threads is that separated Imath is new, why don't we move forward as much as possible at this point. If people really need the legacy functions, maybe we could instead provide ImathLegacy.h and stick a 2022 best before date in it, and not include it from any of our own code?

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Contributor Author

lgritz commented Nov 20, 2020

I've augmented this PR to define IMATH_DEPRECATED in ImathConfig.h, and used it on these deprecated functions.

Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

lgtm

//
#if defined(__cplusplus) && (__cplusplus >= 201402L)
# define IMATH_DEPRECATED(msg) [[deprecated(msg)]]
#elif defined(_MSC_VER)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's so nice to see that _MSC_VER doesn't have to be detected early to bypass the historically broken __cplusplus check!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, well when you say it like that, I wonder if I've done it wrong. There are some compilers that don't like

#if defined(FOO) && ...expression involving FOO...

and I never quite remember when that's allowed. It's probably MSVC with the issue, right?

Perhaps I'll reorder this to be safe.

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 might depend on exactly which MSVC version it is. Hmmm. I'll fix to be sure it's 100% safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to be safer. Also caught one more file where things should have been changed to std::. Grep shows that's the last of it.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
@meshula
Copy link
Contributor

meshula commented Nov 23, 2020

Microsoft being Microsoft, I'm sure the new ordering will be fine until the end of time.

@lgritz lgritz merged commit c9120fd into AcademySoftwareFoundation:master Dec 5, 2020
@lgritz lgritz deleted the lg-stdmath branch December 5, 2020 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants