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

<xutility>: SFINAE constraint on construct_at prevents emplacing immovable objects with copy elision #2620

Closed
Chlorie opened this issue Mar 28, 2022 · 11 comments · Fixed by #2624
Labels
bug Something isn't working compiler Compiler work involved fixed Something works now, yay!

Comments

@Chlorie
Copy link

Chlorie commented Mar 28, 2022

Describe the bug
Currently the constraint on construct_at is implemented via SFINAE

STL/stl/inc/xutility

Lines 132 to 134 in f099e9c

template <class _Ty, class... _Types,
class = void_t<decltype(::new (_STD declval<void*>()) _Ty(_STD declval<_Types>()...))>>
constexpr _Ty* construct_at(_Ty* const _Location, _Types&&... _Args) noexcept(

This causes an error when trying to emplace immovable objects with copy elision:
xutility(145): error C2783: '_Ty *std::construct_at(_Ty *const ,_Types &&...) noexcept(<expr>)': could not deduce template argument for '<unnamed-symbol>'

Example code:

struct S
{
    int v;
    S(int v): v(v) {}
    S(const S&) = delete;
};

union U
{
    char c;
    S s;
    U(): c{} {}
    ~U() noexcept {}
};

struct copy_elider
{
    operator S() const { return S(42); }
};

int main()
{
    U u;
    // AFAIK this should work since C++17 mandates copy elision
    std::construct_at(&u.s, copy_elider{});
}

Yet if the constraint is implemented via concepts, i.e.

template <class _Ty, class... _Types>
    requires requires (void* ptr, _Types&&... _Args) { ::new (ptr) _Ty(static_cast<_Types&&>(_Args)...); }
constexpr _Ty* construct_at(_Ty* const _Location, _Types&&... _Args) noexcept(

then the example code above works as expected. I don't know the reason that these two behaviors differ though...

This issue hinders the ability to emplace immovable objects into optionals or variants using the copy elision converter trick shown above.

STL version
VS Community 2022, Version 17.1.2

@miscco
Copy link
Contributor

miscco commented Mar 28, 2022

Originally we had it constrained via a requires clause, but that lead to some strange compiler errors for some corner cases. (Not to speak of intellisense which is EDG based and thus currently not available for concepts)

@StephanTLavavej found this to be the appropriate perma workaround. Do you still remember what broke?

@miscco
Copy link
Contributor

miscco commented Mar 28, 2022

Oh and it is constexpr container support machinery. Who would have thought?

#1546 (comment)

Fixed by 075dfaf

I see that the original bug was fixed, so maybe we can go back to the proper constraint?

@miscco
Copy link
Contributor

miscco commented Mar 28, 2022

Also please note, that your code exhibits a certain amount of UB, because you are taking the reference to a not yet alive object.

std::construct_at(&u.s, copy_elider{});

here u.s is not yet alive so taking a reference is ill formed. you should be fine, but do not expect this to work in a constexpr context

@Chlorie
Copy link
Author

Chlorie commented Mar 28, 2022

Also please note, that your code exhibits a certain amount of UB, because you are taking the reference to a not yet alive object.

std::construct_at(&u.s, copy_elider{});

here u.s is not yet alive so taking a reference is ill formed. you should be fine, but do not expect this to work in a constexpr context

Yes, I'm aware of the potential UB. It's just that I couldn't come up with any concise examples without UB 😭
I stumbled upon the issue when trying to emplace immovable objects into optionals as described in the main description.

@frederick-vs-ja
Copy link
Contributor

Also please note, that your code exhibits a certain amount of UB, because you are taking the reference to a not yet alive object.

std::construct_at(&u.s, copy_elider{});

here u.s is not yet alive so taking a reference is ill formed. you should be fine, but do not expect this to work in a constexpr context

No, this is well-defined, as at least the code is just taking the address of an inactive member. If such code has UB, then we can't generally reasonably change active member of a union during constant evaluation.

Using std::addressof in such case may be UB. This is WG21-CWG453.

@miscco
Copy link
Contributor

miscco commented Mar 28, 2022

Ahh, sorry, you are correct, you can take the address. https://godbolt.org/z/zbbWPr3M3

The issue I was miss-remembering was that we passed that address on into another function (char_traits for std::string) and there it is indeed invalid.

@fsb4000
Copy link
Contributor

fsb4000 commented Mar 28, 2022

C:/data/msvc/14.31.31104/include\xmemory(676): error C2783: '_Ty *std::construct_at(_Ty *const ,_Types &&...) noexcept(<expr>)': could not deduce template argument for '<unnamed-symbol>'

I saw the same error there: DevCom-1691516 and I think it might be a compiler issue.
Because your example (and DevCom-1691516 example) works fine with clang-cl and Microsoft STL.

@StephanTLavavej StephanTLavavej added bug Something isn't working compiler Compiler work involved labels Mar 28, 2022
@StephanTLavavej
Copy link
Member

Agreed that this seems like a compiler issue if it works fine with Clang and MSVC's STL (haven't confirmed), labeling accordingly.

@frederick-vs-ja
Copy link
Contributor

It seems that MSVC works if implementation of the constraint is changed to:

void_t<decltype(::new (_STD declval<void*>()) _Ty(_STD declval<_Types>()...))>* = nullptr

@fsb4000
Copy link
Contributor

fsb4000 commented Mar 29, 2022

@frederick-vs-ja you are correct C1XX and Clang work with your solution but unfortunately EDG doesn't work with both constraints (your and initial) :(

изображение

Do you think we need to somehow reduce the example or just report it as it is to DevCom?

@miscco
Copy link
Contributor

miscco commented Mar 29, 2022

I think we should report that issue for EDG and work around it by keeping the original code for EDG with #ifdef __EDG__ and using the fixed version in other cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler Compiler work involved fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants