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

Add a compatibility fix for C++20 #69

Merged
merged 1 commit into from
May 13, 2024

Conversation

irieger
Copy link
Contributor

@irieger irieger commented May 10, 2024

Add a compatibility fix for C++20 (and maybe others) to fix a non-supported conversion from std::string_view to std::string.

When preparing a recipe for the Conan package manager to properly add bmx as a dependency in other projects. One of the points there is to honor the user request for the C++ version requested by the user, thus I patch out the fixed setting of C++11. (See corresponding PR: conan-io/conan-center-index#23955)

With C++20 set, I got the following error requiring an implicit conversion:

/home/ingmar/.conan2/p/b/bmx8699e87e1c8ea/b/src/src/common/Version.cpp: In function ‘mxfProductVersion bmx::get_bmx_mxf_product_version()’:
/home/ingmar/.conan2/p/b/bmx8699e87e1c8ea/b/src/src/common/Version.cpp:222:47: error: conversion from ‘const bmx_git::StringOrView’ {aka ‘const std::basic_string_view<char>’} to non-scalar type ‘std::string’ {aka ‘std::__cxx11::basic_string<char>’} requested
  222 |         string describe = bmx_git::DescribeTag();

@philipnbbc
Copy link
Collaborator

Thanks for looking into this. I set CMAKE_CXX_STANDARD to 20 in the libMXF and libMXF++ libraries as well and found one more issue with a missing include - see below. If you could add that one as well please then it should all be good.

--- a/deps/libMXFpp/libMXF++/MXFVersion.cpp
+++ b/deps/libMXFpp/libMXF++/MXFVersion.cpp
@@ -33,6 +33,8 @@
 #include "config.h"
 #endif
 
+#include <string>
+
 #include "git.h"
 #include "fallback_git_version.h"

@philipnbbc philipnbbc self-requested a review May 13, 2024 08:22
…upported conversion from std::string_view

and add missing include for MXFVersion.cpp.
@irieger irieger force-pushed the cpp20-compatibility-patch branch from 3209e27 to 957960b Compare May 13, 2024 10:48
@irieger
Copy link
Contributor Author

irieger commented May 13, 2024

Thanks for looking into this. I set CMAKE_CXX_STANDARD to 20 in the libMXF and libMXF++ libraries as well and found one more issue with a missing include - see below. If you could add that one as well please then it should all be good.

--- a/deps/libMXFpp/libMXF++/MXFVersion.cpp
+++ b/deps/libMXFpp/libMXF++/MXFVersion.cpp
@@ -33,6 +33,8 @@
 #include "config.h"
 #endif
 
+#include <string>
+
 #include "git.h"
 #include "fallback_git_version.h"

Sure, just added it. And with your hint also changed our patches for Conan to comment out set(CMAKE_CXX_STANDARD 11) also for the deps and tried it with that patch.

Copy link
Collaborator

@philipnbbc philipnbbc left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@philipnbbc philipnbbc merged commit 6e64980 into bbc:main May 13, 2024
5 checks passed
@irieger irieger deleted the cpp20-compatibility-patch branch May 13, 2024 21:06
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.

None yet

2 participants