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

Deprecation of non-floating complex #889

Closed

Conversation

AlexGuteniev
Copy link
Contributor

Deprecation of nonstandard extension mentioned in #756

@AlexGuteniev
Copy link
Contributor Author

So here are unexpected failures:

: Failing Tests (12):
1:   libc++ :: std/numerics/complex.number/cmplx.over/real.pass.cpp:0
1:   libc++ :: std/numerics/complex.number/cmplx.over/imag.pass.cpp:0
1:   libc++ :: std/numerics/complex.number/cmplx.over/real.pass.cpp:1
1:   libc++ :: std/numerics/complex.number/complex.member.ops/times_equal_complex.pass.cpp:0
1:   libc++ :: std/numerics/complex.number/complex.member.ops/times_equal_complex.pass.cpp:1
1:   libc++ :: std/numerics/complex.number/complex.member.ops/minus_equal_complex.pass.cpp:0
1:   libc++ :: std/numerics/complex.number/cmplx.over/imag.pass.cpp:1
1:   libc++ :: std/numerics/complex.number/complex.member.ops/plus_equal_complex.pass.cpp:0
1:   libc++ :: std/numerics/complex.number/complex.member.ops/plus_equal_complex.pass.cpp:1
1:   libc++ :: std/numerics/complex.number/complex.members/real_imag.pass.cpp:0
1:   libc++ :: std/numerics/complex.number/complex.members/real_imag.pass.cpp:1
1:   libc++ :: std/numerics/complex.number/complex.member.ops/minus_equal_complex.pass.cpp:1

I'm trying to fix them by passing /D_SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING to cl when running libcxx tests.

@AlexGuteniev
Copy link
Contributor Author

Now test pass. Interestingly. complex<int> extension is only covered by libcxx test.

@CaseyCarter CaseyCarter added decision needed We need to choose something before working on this enhancement Something can be improved labels Jun 10, 2020
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

It's not clear to me that we want to deprecate this functionality. Do you have any motivating arguments?

@@ -3,7 +3,7 @@

RUNALL_INCLUDE ..\universal_prefix.lst
RUNALL_CROSSLIST
PM_CL="/EHsc /MTd /std:c++latest /permissive- /FImsvc_stdlib_force_include.h /wd4643"
PM_CL="/EHsc /MTd /std:c++latest /permissive- /FImsvc_stdlib_force_include.h /D_SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING /wd4643"
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically add deprecation suppression defines to msvc_stdlib_force_include.h to keep the command line from growing without bound. (I wouldn't put any effort into upstreaming that change until we decide if we want this deprecation.)

Copy link
Contributor

Choose a reason for hiding this comment

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

We can push this change into LLVM after we otherwise finish review of the PR. I'd hate for someone to make the change and then have to do so again if we decide to change the name of the suppression macro.

Copy link
Contributor

@CaseyCarter CaseyCarter Jun 16, 2020

Choose a reason for hiding this comment

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

Upon further consideration, I don't want to block this waiting for the ranges::is_permutation PR (which includes the pertinent LLVM changes) to merge. I'd be happy to merge this as-is with an issue to move this macro definition from the command line into the header once we're able to update LLVM again.

Co-authored-by: Casey Carter <[email protected]>
@AlexGuteniev
Copy link
Contributor Author

It is not possible to implement many operations for complex<int> properly.
@statementreply observed that sqrt(complex<int>{1, 0}) crashes, for instance.

@StephanTLavavej
Copy link
Member

The Standard says that the effects are unspecified, so code using complex<int> isn't portable. I think deprecation is justified because (1) stuff doesn't work as a user might expect (thanks @statementreply and @AlexGuteniev for the example) and (2) this is an ongoing implementation/test cost. For similar reasons, we deprecated and removed <allocators> (although the concerns were more intense there).

Mostly I'm not sure how much usage has accumulated in the last 20+ years, but this is just deprecation with an escape hatch, not removal.

@CaseyCarter CaseyCarter removed the decision needed We need to choose something before working on this label Jun 12, 2020
@CaseyCarter
Copy link
Contributor

We discussed this during our weekly meeting and decided we saw no reason not to deprecate, but wanted wait for @StephanTLavavej to return from vacation and possibly provide historical context. The above comment - which was posted shortly before that meeting, but I only noticed just now - is what we were wanting.

I've removed the "decision needed" label so this PR can move forward.

@CaseyCarter CaseyCarter self-requested a review June 12, 2020 22:26
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good; I'll push a change to qualify std::complex in the deprecation message.

@StephanTLavavej
Copy link
Member

Unfortunately, this deprecation warning is causing our "Real World Code" test suite to fail, so I wasn't able to merge this. Either NVIDIA/cutlass#104 needs to be fixed upstream, or we need to figure out how to properly silence this warning in our Microsoft-internal infrastructure (I tried adding a compiler option to define the appropriate macro in our build script via CMake, but it didn't work as I expected, and I ran out of time to continue investigating). 😿

@StephanTLavavej StephanTLavavej removed their assignment Jun 26, 2020
@StephanTLavavej
Copy link
Member

NVIDIA/cutlass#104 is still active upstream. I may be able to figure out how to patch their sources in our RWC suite, which should be more effective than attempting to add the compiler option (I've successfully patched code before), but I won't have time to investigate that in the near future.

@AlexGuteniev
Copy link
Contributor Author

Do you want me to close this PR, convert it to draft, or keep open?

@StephanTLavavej
Copy link
Member

I think it was worth a try, but given the effort we've discovered this will take to ship in production, against the possible user benefit (which I think is minimal, even though I am a huge fan of deprecation in general), I would prefer to close this PR; this will free up time for features and more important improvements. However, I would welcome a vNext enhancement issue to simply remove the primary template (that's a perfect kind of vNext issue since the product change is so simple).

@AlexGuteniev AlexGuteniev deleted the deprecate_primary_complex branch July 23, 2020 10:43
@AlexGuteniev AlexGuteniev restored the deprecate_primary_complex branch June 4, 2022 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something is preventing work on this enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants