Skip to content

Conversation

Radnyx
Copy link

@Radnyx Radnyx commented Oct 8, 2025

MSVC is knocking out bugs to get mp-units building. One of those bugs occurs here in unit.h:

template<typename... Us, Unit U>
[[nodiscard]] consteval Unit auto get_common_unit_in(common_unit<Us...>, U u)
{
  // ...
  constexpr auto canonical_u = get_canonical_unit(u);
  // ...
}

The call to get_canonical_unit tries to copy u, which is outside the evaluation context of canonical_u. Normally this would be an error, but since Unit types have no members to copy (they are "empty"), this is actually permitted by the C++ standard.

We get this wrong due to an age-old layout problem. The structs scaled_unit_impl and derived_unit_impl inherit from empty base classes, so they should be considered empty as well. But due to historical reasons, we don't grant this if there are multiple base classes.

This is fixable on the compiler side. In the meantime, adding __declspec(empty_bases) is the workaround to get standard behavior (MSVC docs, introduced in Visual Studio 2015).

Copy link
Owner

@mpusz mpusz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I wasn't aware of it and had to fix many similar lines in mp-units before to make MSVC happier. I was changing u to U{} and similar everywhere. Units are not the only ones affected. Similar issues may happen with dimension and quantity_spec classes, as they have the same design and usage. If you didn't approach problems so far, it means that I've probably fixed most of those already 😉

If only I had known about this workaround, it would have been much easier to get the MSVC compliance earlier.

@mpusz mpusz merged commit 4b933b4 into mpusz:master Oct 9, 2025
90 checks passed
@mpusz
Copy link
Owner

mpusz commented Oct 9, 2025

BTW, I really appreciate the MSVC developers' efforts here! 💜

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 this pull request may close these issues.

2 participants