Fix `empty_value` MSVC bug

[k3DW]

See unordered#257.

Is this a breaking change in Core? It doesn't break any tests, but is it conceptually wrong?

[mglisse]

(thank you for working on this. Just a user's comments below)

Is this a breaking change in Core?

I think so. As broken as the permission model is in C++ (private things are traps instead of being invisible), there are still some cases where SFINAE can ignore private things, that work with the current code and would be broken with public inheritance.

is it conceptually wrong?

Again, I think so. It may still be worth it if the bug it works around is worse than what this breaks (I have no idea how to measure that), but if this change is done, in my opinion it should be protected by something like BOOST_WORKAROUND(BOOST_MSVC, ...) with a link to the bug report on Microsoft's website.

On recent compilers, a simpler implementation of empty_value based on (msvc::)no_unique_address could also be considered.

[pdimov]

On recent compilers, a simpler implementation of empty_value based on (msvc::)no_unique_address could also be considered.

This sounds like the best course of action here.

[k3DW]

On recent compilers, a simpler implementation of empty_value based on (msvc::)no_unique_address could also be considered.

This sounds like the best course of action here.

I'm not sure how far back this problem goes on MSVC. On 16.0, the oldest available version on Compiler Explorer right now, the issue reproduces.

That said, how would this be implemented using [[msvc::no_unique_address]]? Usages of empty_value rely on EBO, which is transitive through inheritance. Anything using NUA isn't transitive. CE link

I can't find any documentation for when [[msvc::no_unique_address]] was introduced. This attribute doesn't work in MSVC 16.8, but it starts working in MSVC 16.9. We need this fixed earlier than 16.8, so I don't think we can use NUA here.

[pdimov]

[[no_unique_address]] works for the same example under Clang and GCC: https://godbolt.org/z/PzqreT8Eb But we need it for MSVC, which is exactly where it doesn't. :-)

[Lastique]

Was this bug reported to MSVC devs? If not, please do.

As to this PR, I think, this is a better workaround. [[no_unique_address]] is not worth it. And not the right way to go anyway, as compilers are allowed to ignore/not support attributes, while EBO is a requirement.

[mglisse]

That said, how would this be implemented using [[msvc::no_unique_address]]? Usages of empty_value rely on EBO, which is transitive through inheritance. Anything using NUA isn't transitive. CE link

:exploding_head: The whole point of NUA is to replace EBO, and MSVC managed to fail...

Actually, msvc::no_unique_address is a failure, but there may still be a bit of hope for the real no_unique_address if we complain enough (report 1, report 2) before the next ABI break in MSVC :crossed_fingers:

[Lastique]

Thanks. Looks ok to me.

[k3DW]

Thanks. Looks ok to me.

Great thanks

Was this bug reported to MSVC devs? If not, please do.

Got it, here is the bug report. It seems like it's been around for a while, so I'm not holding out hope that it'll get fixed any time soon. But at least we were able to get a workaround

[Lastique]

Was this bug reported to MSVC devs? If not, please do.

Got it, here is the bug report.

Please, add a short description and a link to the bug in a comment near #if defined(BOOST_MSVC) and the new test_derived_compile test.

It seems like it's been around for a while, so I'm not holding out hope that it'll get fixed any time soon.

If it's not been reported before, it is likely it wasn't known to the MSVC devs, which is why it wasn't fixed and why it needed to be reported. I don't expect it to be immediately fixed, but it will be, eventually.