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

C++20 #4803

Merged
merged 12 commits into from
Jun 26, 2022
Merged

C++20 #4803

merged 12 commits into from
Jun 26, 2022

Conversation

Swiftb0y
Copy link
Member

Commits contain various fixes for warnings and compile-errors. The last commit actually does the switch.
Tested on GCC12 locally, using CI for everything else.

Swiftb0y added 7 commits June 7, 2022 20:28
…SSIGN

While valid on all other compilers, gcc-11 and newer break when in
C++20 mode. I do not know if this is a bug or if there is really a
meaningfull difference between a "templated copy constructor" and
a "non-templated copy constructor".
C++20 changes some confusing rules about aggregate types and their
initialization. As such, mixxx:network::jsonwebtask is no longer
considered an aggregate in C++20. Thus we provide a proper
constructor for its members.

At the same time, remove explicit `= default` of copy-&move-ctor/
assignment. These are implemented implicitly by the compiler.
@Swiftb0y
Copy link
Member Author

Swiftb0y commented Jun 14, 2022

Arch build fails because of a GCC bug?
google/benchmark#1398
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105329

@uklotzde
Copy link
Contributor

Arch build fails because of a GCC bug? google/benchmark#1398 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105329

These are the bugs I have experienced on RPM Fusion during the Fedora 36 Beta phase.

@Swiftb0y
Copy link
Member Author

This doesn't happen on my F36 machine which has gcc 12.1.1? Where does the last .1 come from? Does fedora patch gcc themselves or do they do a rolling release or what is happening here?

@daschuer
Copy link
Member

CMake is smart enough to pass "-std=gnu++2a" to Ubuntu Focal GCC Version 9.4.0
This works for me. Thank you.

Swiftb0y and others added 2 commits June 15, 2022 12:44
Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>
refactor(autodj): remove m_pPlayerManager from autodjprocessor

Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>
@Swiftb0y
Copy link
Member Author

Done. Let me autosquash before you merge.

@daschuer
Copy link
Member

The Arch Linux issue is:

Error: /usr/include/c++/12.1.0/bits/char_traits.h:431:56: error: ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’ accessing 9223372036854775810 or more bytes at offsets -4611686018427387902 and [-4611686018427387903, 4611686018427387904] may overlap up to 9223372036854775813 bytes at offset -3 [-Werror=restrict]
  431 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
      |                                        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

Do we have an idea to fix it?

@Swiftb0y
Copy link
Member Author

Swiftb0y commented Jun 15, 2022

Not our fault: #4803 (comment)

Its a regression thats only present on GCC 12.1 (edit: maybe 12.0 as well) so we'll just have to wait a bit and upgrade the arch docker container.

@uklotzde
Copy link
Contributor

Just noticed that I haven't backported the patch from RPM Fusion that is still needed: https://github.com/rpmfusion/mixxx/blob/master/disable_werror_in_tests.patch

@Swiftb0y
Copy link
Member Author

I guess that makes sense why the fedora builds succeed while arch doesn't even though they use the same compiler...

@uklotzde
Copy link
Contributor

I guess that makes sense why the fedora builds succeed while arch doesn't even though they use the same compiler...

Only fails with rpmbuild. Manual CMake builds are not affected.

@Swiftb0y
Copy link
Member Author

So should be disable the buggy warnings in our vendored google benchmark?

@Swiftb0y
Copy link
Member Author

@uklotzde can you give me a hint of how to do that? Should I simply just commit the changes to the CMakeLists of google benchmark? Do I supply the .patch from git? Do you instead overwrite some compiler defines in our CMakeLists.txt?

add_cxx_compiler_flag(-Werror RELEASE)
add_cxx_compiler_flag(-Werror RELWITHDEBINFO)
add_cxx_compiler_flag(-Werror MINSIZEREL)
# Disable -Werror because of gcc bug
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok to simply inline the patch here. It's only a workaround that should hopefully become obsolete in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't quite understand. So its fine as is or should I also commit a (imo redundant) patch file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how soon this will be fixed though. Neither google seems to make any effort working around this issue, nor the gcc team sees that as very urgent...

Copy link
Contributor

Choose a reason for hiding this comment

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

All fine. Hopefully, we don't need to update the lib anytime soon.

@Swiftb0y
Copy link
Member Author

Great, @daschuer merge?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

This still works for me on Ubuntu Focal. Thank you.

@daschuer daschuer merged commit d209239 into mixxxdj:main Jun 26, 2022
@Swiftb0y Swiftb0y deleted the cpp20 branch June 26, 2022 21:07
This was referenced Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants