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

std::tie does not work with derived classes under libc++ #578

Closed
lemire opened this issue Mar 21, 2020 · 15 comments
Closed

std::tie does not work with derived classes under libc++ #578

lemire opened this issue Mar 21, 2020 · 15 comments

Comments

@lemire
Copy link
Member

lemire commented Mar 21, 2020

Apple has its own version of LLVM's clang which should follow closely clang, but often differs slightly. Unfortunately, many programmers are mac users so it is inconvenient to leave macOS users in the cold.

The following won't build under macOS:

#include <tuple>
struct t : std::tuple<int,float,char> {};
int main () {
int myint; char mychar;
t tt;
std::tie (myint, std::ignore, mychar) = tt;
}

The error is as follows...

$ g++ -o crss crss.cpp  -std=c++17
crss.cpp:11:41: error: no viable overloaded '='
  std::tie (myint, std::ignore, mychar) = tt;   // unpacking tuple into ...
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/tuple:872:12: note:
      candidate function not viable: no known conversion from 't' to 'const
      typename conditional<_CanCopyAssign::value, tuple<int &, const
      __ignore_t<unsigned char> &, char &>, __nat>::type' (aka 'const
      std::__1::tuple<int &, const std::__1::__ignore_t<unsigned char> &, char
      &>') for 1st argument
    tuple& operator=(typename conditional<_CanCopyAssign::value, tuple, ...
           ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/tuple:880:12: note:
      candidate function not viable: no known conversion from 't' to 'typename
      conditional<_CanMoveAssign::value, tuple<int &, const __ignore_t<unsigned
      char> &, char &>, __nat>::type' (aka 'std::__1::tuple<int &, const
      std::__1::__ignore_t<unsigned char> &, char &>') for 1st argument
    tuple& operator=(typename conditional<_CanMoveAssign::value, tuple, ...
           ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/tuple:895:9: note:
      candidate template ignored: requirement '__tuple_assignable<t &,
      std::__1::tuple<int &, const std::__1::__ignore_t<unsigned char> &, char
      &>, false, true>::value' was not satisfied [with _Tuple = t &]
        operator=(_Tuple&& __t) _NOEXCEPT_((is_nothrow_assignable<_BaseT...
        ^
1 error generated.

cc @jkeiser

@lemire
Copy link
Member Author

lemire commented Mar 21, 2020

The following works fine...

#include <tuple>
using t = std::tuple<int,float,char>;
int main ()
{
  int myint;
  char mychar;
  t tt;
  std::tie (myint, std::ignore, mychar) = tt;   // unpacking tuple into variables
}

@lemire
Copy link
Member Author

lemire commented Mar 21, 2020

Ideas/workarounds are invited.

@lemire
Copy link
Member Author

lemire commented Mar 21, 2020

What is worse is that I do not know how to test this in CI. I only found out because I sometimes run tests on my macbook.

@beached
Copy link

beached commented Mar 21, 2020

This is libc++ https://gcc.godbolt.org/z/VH55tH However, it's not tie but something in libc++'s tuple I think https://gcc.godbolt.org/z/csVXTE

@ThePhD
Copy link

ThePhD commented Mar 21, 2020

The problem:
https://twitter.com/thephantomderp/status/1241207294953693184

The fix (Godbolt):

#include <tuple>

using t_base = std::tuple<int, float, char>;

struct t : t_base {};

int main() {
  int myint;
  char mychar;
  t tt;
  std::tie(myint, std::ignore, mychar) = static_cast<t_base&>(tt);
}

The longer fix:
Tell libc++ to get good. Their library isn't compliant with the Standard, which lists overloads which explicitly take const& and && qualified tuples; that means derived classes with public std::tuple bases should work with those operators, and so should classes with conversion operators.

@lemire
Copy link
Member Author

lemire commented Mar 21, 2020

Thank you folks. So we have identified the true source of the problem: libc++. This should make testing easier, at the least.

@ThePhD's workaround (a cast to pair) can probably be made to look nice with some effort.

@lemire
Copy link
Member Author

lemire commented Mar 21, 2020

We are now going to test with libc++ in CI.

See #579

@lemire lemire changed the title std::tie does not work with derived classes Apple clang version 11.0.0 std::tie does not work with derived classes under libc++ Mar 21, 2020
@lemire
Copy link
Member Author

lemire commented Mar 21, 2020

The bug is already open in libc++:

https://reviews.llvm.org/D50106

There is a patch but it has been reviewed and blocked in January 2019 (over a year ago):

https://reviews.llvm.org/D50106

I see no workaround being suggested.

@lemire
Copy link
Member Author

lemire commented Mar 21, 2020

@ldionne can you help us?

@jkeiser
Copy link
Member

jkeiser commented Mar 22, 2020

The two things I'm wondering:

  • Is there an operator std::pair<X,Y> or something we can add to simdjson_result to fix it?
  • Can we directly implement __tuple_assignable or something when compiling against libc++, to avoid this error?
    candidate template ignored: requirement '__tuple_assignable<t &, std::__1::tuple<int &, const std::__1::__ignore_t<unsigned char> &, char &>, false, true>::value' was not satisfied [with _Tuple = t &] operator=(_Tuple&& __t) _NOEXCEPT_((is_nothrow_assignable<_BaseT...
    

Actually making simdjson_result == std::pair is possible but because we add methods to the class, I think we'd have to completely redefine it for that variant (e.g. class std::pair<document, error_code>{ ... simdjson_result<document::array> as_array(); ... }). That, or abandon error chaining, which is the reason for the methods.

@lemire
Copy link
Member Author

lemire commented Mar 22, 2020

@jkeiser

My current understanding of C++ is that assignment operators cannot be static, they have to be defined in the class. In other programming languages like Swift, you can extend a class from the outside, so it would not be a problem, but in C++ you cannot. So if you want to redefine how this get compiled...

a = b;

... you have to have some control on the class 'a' belongs to. And we do not.

So I don't see how we can fix this by hacking the operator assignment.

@lemire
Copy link
Member Author

lemire commented Mar 27, 2020

@jkeiser Maybe we can close this... I don't think that the libc++ folks are going to be rushing in to offer workarounds...

@lemire
Copy link
Member Author

lemire commented Mar 28, 2020

Ok. I am closing this issue.

@lemire lemire closed this as completed Mar 28, 2020
@ldionne
Copy link

ldionne commented Mar 18, 2021

FWIW we fixed this in:

commit a0839b14df6de99fe29bee7cdfff182d50de665d
Author: Louis Dionne <ldionne@apple.com>
Date:   Tue Jul 31 11:56:20 2018 -0400

    [libc++] Fix tuple assignment from types derived from a tuple-like

    The implementation of tuple's constructors and assignment operators
    currently diverges from the way the Standard specifies them, which leads
    to subtle cases where the behavior is not as specified. In particular, a
    class derived from a tuple-like type (e.g. pair) can't be assigned to a
    tuple with corresponding members, when it should. This commit re-implements
    the assignment operators (BUT NOT THE CONSTRUCTORS) in a way much closer
    to the specification to get rid of this bug. Most of the tests have been
    stolen from Eric's patch https://reviews.llvm.org/D27606.

    As a fly-by improvement, tests for noexcept correctness have been added
    to all overloads of operator=. We should tackle the same issue for the
    tuple constructors in a future patch - I'm just trying to make progress
    on fixing this long-standing bug.

    PR17550
    rdar://15837420

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

@lemire
Copy link
Member Author

lemire commented Mar 18, 2021

Merci @ldionne

C'était vraiment énervant. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants