mame icon indicating copy to clipboard operation
mame copied to clipboard

fix vs2019_clang

Open vadosnaprimer opened this issue 1 year ago • 5 comments

BGFX fix was done by taking those lines from current BGFX upstream.

emu_fatalerror fix was done by @SimonN who would be able to explain it way better than me (in a few hours).

vadosnaprimer avatar Dec 18 '23 20:12 vadosnaprimer

Edit 2024-06-03: The information below the line in this post isn't necessary anymore. In December 2023, this PR had two parts: The BGFX modifications (still in here) and a build error fix for Clang (this is now removed). The build error fix has become unnecessary after #12383 got merged, which fixes the same error in a different way.

Here is my original explanation from December 2023 about the (now removed) build error fix for Clang:


This PR makes MAME build with Clang (clang-cl) in Visual Studio.

The problem was: When a mutable emu_fatalerror is to be copied, the copy constructor was not the best match; it takes by reference-to-const. Instead, the compiler correctly chose the templatized constructor because that's an even better match: It takes by <T> (T &&), which instantiates to (emu_fatalerror &) (reference-to-mutable). The body of the template then fails to compile; e.g., this T has no member empty, but that's too late to re-choose a different constructor.

This fix introduces another overload for the constructor. The new overload takes specifically by mutable reference and delegates to the already-working copy constructor. Compilers prefer this overload over the templatized constructor.

The idea came from the discussion in #11316: Different compilers instantiate exceptions in different ways. Some compilers apparently create copies of mutable emu_fatalerror and some do not. It's reasonable for all compilers to copy both from mutable and from const. @backrock, this PR should fix the build for you.

Other compilers' opinions about this fix:

  • g++ 13.2 on Linux builds MAME with or without this fix. No problems here.
  • Clang on Linux: We haven't tested this, but it's probably OK because clang-cl likes the fix.
  • MSVC: The build is still broken with MSVC. Before the fix, it gave the same error as clang-cl (T has no member empty), but now the error is:
src\emu\emucore.h(231,2): error C2580:
    'emu_fatalerror::emu_fatalerror(const emu_fatalerror &)':
    multiple versions of a defaulted special member functions are not allowed
    1> emu_fatalerror(emu_fatalerror const &) = default;

Given this error by MSVC, I'm not 100 % sure if the fix is conformant C++17 even though even I believe it is. At least the other compilers are happy.


Rejected alternative ways to fix:

  • Specialize the templatized constructor for emu_fatalerror & and again forward to the copy constructor. This works in principle because you are allowed to specialize a <T> X(T &&) (two ampersands) to X(X&) (one ampersand). That works well for this particular class emu_fatalerror. But other classes in the codebase (see below) with the same problem are templates themselves, and you must declare specializations in namespace scope, i.e., below the class. Even if it's possible to get that syntax right, it will be harder to read than a non-template overload inside the class declaration.
  • Constrain the templatized constructor to not take any kind of reference to emu_fatalerror. That needs C++20 concepts, but MAME's build instructions require C++17.

Adding explicit to the templatized constructor did not fix the build on clang-cl, even though it allegedly works with MSVC. Still, explicit will be a good idea on its own.

Unrelated to this particular fixed build in Clang: We found other classes in the codebase, e.g., format_argument_pack and format_argument_pack_impl, that can theoretically run into the same problem. They offer a copy constructor X(const X &) and a templatized constructor <T, Args...> X(T &&, Args... &&). When a mutable class object is to be copied, the templatized constructor is the best match (reference-to-mutable, and zero Args). For long-term maintenance, it may make sense to overload the constructor there, i.e., add:

X(X &x) : X(static_cast<X const &>(x) {}

But that's out of scope for this PR. This PR is a minimal fix to build in clang-cl.

SimonN avatar Dec 19 '23 06:12 SimonN

thanks, so good.

backrock avatar Dec 19 '23 13:12 backrock

I submitted a different fix for the emu_fatalerror issue: #12383. I haven't tested with Clang, but I see no reason why it wouldn't work with it.

SpecLad avatar May 17 '24 22:05 SpecLad

With #12383 merged, our PR here doesn't need to address the constructor overloading anymore. (Theoretically, the codebase should work fine with both fixes, but it would be unnecessary complication to have both fixes.)

@vadosnaprimer, I recommend:

  1. Remove our changes in src/emu/emucore.h. Our PR should touch only 3rdparty/bgfx/src/renderer_d3d12.cpp.
  2. Rebase the PR onto upstream's master to make merging easier.
  3. While we're at it: Are your BGFX fixes still up-to-date? Are they still relevant?

SimonN avatar Jun 03 '24 10:06 SimonN

Thanks for the reminder!

BGFX hasn't been updated since last year, and doesn't currently have the fix for clang-cl.

Regular MSVC builds it fine as well now.

vadosnaprimer avatar Jun 03 '24 16:06 vadosnaprimer