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

static cast a derived class to base class with defined type case operators #37949

Closed
llvmbot opened this issue Aug 16, 2018 · 8 comments
Closed
Assignees
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 16, 2018

Bugzilla Link 38601
Resolution FIXED
Resolved on Jun 08, 2021 08:36
Version 7.0
OS MacOS X
Reporter LLVM Bugzilla Contributor
CC @dwblaikie,@ldionne,@mclow,@zygoloid
Fixed by commit(s) a3ab512

Extended Description

#include
#include

using Base = std::tuple<int,int>;

struct Derived : Base {

template <class ...Ts>
Derived(int x, Ts&&... ts): Base(std::forward<Ts>(ts)...), x(x) {
}

operator int () const {
    return x;
}

int x;

};

int main() {

Derived d(1, 2, 3);

Base b = static_cast<Base>(d);

// he output is 0 1 in clang++, but the expected output is 2 3
std::cout << std::get<0>(b) << " " << std::get<1>(b) << std::endl;

return 0;

}

@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 16, 2018

assigned to @ldionne

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented Aug 16, 2018

This happens because libc++ (as an extension) permits a std::tuple<int, int> to be constructed from a single int, with the second int being default-constructed. static_cast(d) in this case performs the same initialization as

Base b(d);

... which can choose either the tuple copy/move constructor, or the undersized tuple constructor from tuple elements. The former requires a derived-to-base conversion on d, but the latter has 'exact match' rank because the construct-from-elements constructor employs perfect forwarding (the user-defined conversion to int happens inside the constructor, not as part of the call).

So this is a libc++ bug: this is a case where its undersized tuple constructor is non-conforming.

@ldionne
Copy link
Member

ldionne commented Aug 17, 2018

I'm sick and tired of our tuple extensions :(

I wish we could just ship a strictly conforming std::tuple -- it's already hard enough.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 17, 2018

Lets go ahead and remove them then.

I can write a paper later proposing them so we can provide them while being standards conforming.

@ldionne
Copy link
Member

ldionne commented Aug 17, 2018

Do you mean remove-remove, or remove but keep the ability to enable like it has been done for the implicit-constructor-with-reduced-arity extension?

@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 17, 2018

I think remove remove, I don't know if anyone actually re-enables the extension.

@mclow
Copy link
Contributor

mclow commented Aug 17, 2018

Remove and see what breaks :-(

@ldionne
Copy link
Member

ldionne commented Jun 8, 2021

Fixed by

commit a3ab512
Author: Louis Dionne ldionne.2@gmail.com
Date: Wed Feb 10 16:19:50 2021 -0500

[libc++] Rewrite the tuple constructors to be strictly Standards conforming

This nasty patch rewrites the tuple constructors to match those defined
by the Standard. We were previously providing several extensions in those
constructors - those extensions are removed by this patch.

The issue with those extensions is that we've had numerous bugs filed
against us over the years for problems essentially caused by them. As a
result, people are unable to use tuple in ways that are blessed by the
Standard, all that for the perceived benefit of providing them extensions
that they never asked for.

Since this is an API break, I communicated it in the release notes.
I do not foresee major issues with this break because I don't think the
extensions are too widely relied upon, but we can ship it and see if we
get complaints before the next LLVM release - that will give us some
amount of information regarding how much use these extensions have.

Differential Revision: https://reviews.llvm.org/D96523

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

No branches or pull requests

3 participants