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

<locale>: locale's copy assignment has incorrect return type #268

Closed
CaseyCarter opened this issue Nov 6, 2019 · 6 comments · Fixed by #324
Closed

<locale>: locale's copy assignment has incorrect return type #268

CaseyCarter opened this issue Nov 6, 2019 · 6 comments · Fixed by #324
Labels
bug Something isn't working fixed Something works now, yay! LWG issue needed A wording defect that should be submitted to LWG as a new issue

Comments

@CaseyCarter
Copy link
Member

CaseyCarter commented Nov 6, 2019

Per [locale], std::locale's copy assignment std::locale::operator=(const locale&) should return a constant lvalue that denotes *this. We incorrectly return a non-const lvalue:

locale& operator=(const locale& _Right) noexcept {

Command-line test case
https://godbolt.org/z/bkFbL-

@CaseyCarter CaseyCarter added the bug Something isn't working label Nov 6, 2019
@lozinska
Copy link
Contributor

May I work on this issue?

@StephanTLavavej
Copy link
Member

That's a good question. @CaseyCarter, do you believe that this bug can be fixed while preserving binary compatibility, or is locale's assignment operator separately compiled?

The MSVC-internal build system that we're working on porting to CMake contains automated checks to verify that msvcp140.dll's exported functions aren't changing, but I don't believe that those checks have been ported to CMake yet, hence the need for caution.

The weird calling convention macros like __CLR_OR_THIS_CALL generally indicate separately compiled functions; their absence on this assignment operator leads me to think that changing this will be safe.

@lozinska - I think you can go ahead and submit a PR, and we'll run internal testing to verify it. Thanks for checking!

@CaseyCarter CaseyCarter added the LWG issue needed A wording defect that should be submitted to LWG as a new issue label Nov 15, 2019
@CaseyCarter
Copy link
Member Author

do you believe that this bug can be fixed while preserving binary compatibility, or is locale's assignment operator separately compiled?

It doesn't seem to be - locale isn't tagged dllimport, although some of its member types (id, facet) are.

That said, it's not 100% clear to me if we should correct this discrepancy by changing the return type in our implementation or by changing the return type in the Standard. The const locale& return type is "weird", the only such occurrence I'm aware of in the Standard Library, and precludes locale from satisfying std::copyable which it otherwise would (https://godbolt.org/z/Snc8Nc). I'm far from a <locale> expert, but there seems to be no motivation at all for this choice of return type.

I think I've talked myself into this being "LWG issue needed" rather than "bug". Sorry for the confusion - this came to my attention while reviewing a change to adjacent wording last week and I didn't have time to give it thought other than to simply record the issue. I suppose there's no reason we couldn't fix the bug now - it does seem as simple as adding const - and possibly revert that change later if/when WG21 decides to fix the design.

StephanTLavavej pushed a commit that referenced this issue Dec 5, 2019
Addresses #268 by changing the implementation to match the current Standardese.
@StephanTLavavej StephanTLavavej removed bug Something isn't working work in progress labels Dec 5, 2019
@StephanTLavavej
Copy link
Member

As @lozinska has fixed our implementation to conform to the current Standardese, the remaining work here is to file an LWG issue.

@StephanTLavavej
Copy link
Member

I've submitted an LWG issue with a Proposed Resolution and I'll resolve this when it's posted to the issues list.

@StephanTLavavej StephanTLavavej added bug Something isn't working fixed Something works now, yay! and removed work in progress labels Dec 9, 2019
@StephanTLavavej
Copy link
Member

Filed as LWG-3353.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay! LWG issue needed A wording defect that should be submitted to LWG as a new issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants