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

fix build with musl libc #8628

Closed
wants to merge 1 commit into from

Conversation

iFoundSilentHouse
Copy link

Wesnoth was unable to build with musl.
Error message: error: could not convert 'nullptr' from 'std::nullptr_t' to 'int'
It was for strings 43, 44, 65.
I found the same error but with other program: dotnet/runtime#67763
And after changing 'NULL' to '0' build was successful.

Copy link
Contributor

@stevecotton stevecotton left a comment

Choose a reason for hiding this comment

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

There's a bug in the existing code - these std::optional arguments should default to std::nullopt. AFAICS, they're defaulting to values of zero instead. Going to have to read through #7923 and #7924 to work out exactly what they should do, how to expand the tests, and whether those arguments can change to just passing non-optional arguments instead.

Thanks for drawing attention to it, but as you can see from the length of discussion in #7924, it's not the easiest first issue to start with.

Having said that, the code in this PR may be a good change. I suspect the perfect fix will be to change the NULL to 0 but also remove the std::optional wrappers.

@Wedge009 Wedge009 added the Building Build-time issues. label Mar 26, 2024
@iFoundSilentHouse
Copy link
Author

iFoundSilentHouse commented Mar 26, 2024

Having said that, the code in this PR may be a good change. I suspect the perfect fix will be to change the NULL to 0 but also remove the std::optional wrappers.

Well I can try removing them and build it again, then change it in PR. Should I?

@stevecotton
Copy link
Contributor

Yes please. Sorry for the unclear previous message, still working out what to do.

@iFoundSilentHouse
Copy link
Author

iFoundSilentHouse commented Mar 26, 2024

Yes please. Sorry for the unclear previous message, still working out what to do.

This patch builds fine and, as far as I can understand, it doesn't destroy logic behind ...matches_if_present... functions

diff --git a/src/utils/config_filters.cpp b/src/utils/config_filters.cpp
index f833acd7701..dba7a42c2ff 100644
--- a/src/utils/config_filters.cpp
+++ b/src/utils/config_filters.cpp
@@ -53,7 +53,7 @@ bool utils::config_filters::unsigned_matches_if_present(const config& filter, co
        return in_ranges<int>(cfg[attribute].to_int(0), utils::parse_ranges_unsigned(filter[attribute].str()));
 }

-bool utils::config_filters::int_matches_if_present(const config& filter, const config& cfg, const std::string& attribute, std::optional<int> def)
+bool utils::config_filters::int_matches_if_present(const config& filter, const config& cfg, const std::string& attribute, int def)
 {
        if(!filter.has_attribute(attribute)) {
                return true;
@@ -62,12 +62,11 @@ bool utils::config_filters::int_matches_if_present(const config& filter, const c
                return false;
        }

-       int value_def = def ? (*def) : 0;
-       return in_ranges<int>(cfg[attribute].to_int(value_def), utils::parse_ranges_int(filter[attribute].str()));
+       return in_ranges<int>(cfg[attribute].to_int(def), utils::parse_ranges_int(filter[attribute].str()));
 }

 bool utils::config_filters::int_matches_if_present_or_negative(
-       const config& filter, const config& cfg, const std::string& attribute, const std::string& opposite, std::optional<int> def)
+       const config& filter, const config& cfg, const std::string& attribute, const std::string& opposite, int def)
 {
        if(int_matches_if_present(filter, cfg, attribute, def)) {
                return true;
@@ -79,14 +78,13 @@ bool utils::config_filters::int_matches_if_present_or_negative(
                if(!cfg.has_attribute(opposite) && !def) {
                        return false;
                }
-               int value_def = def ? (*def) : 0;
-               return in_ranges<int>(-cfg[opposite].to_int(value_def), utils::parse_ranges_int(filter[attribute].str()));
+               return in_ranges<int>(-cfg[opposite].to_int(def), utils::parse_ranges_int(filter[attribute].str()));
        }

        return false;
 }

-bool utils::config_filters::double_matches_if_present(const config& filter, const config& cfg, const std::string& attribute, std::optional<double> def)
+bool utils::config_filters::double_matches_if_present(const config& filter, const config& cfg, const std::string& attribute, double def)
 {
        if(!filter.has_attribute(attribute)) {
                return true;
@@ -95,7 +93,7 @@ bool utils::config_filters::double_matches_if_present(const config& filter, cons
                return false;
        }

-       double value_def = def ? (*def) : 1;
+       double value_def = (def==0) ? 1 : (def);
        return in_ranges<double>(cfg[attribute].to_double(value_def), utils::parse_ranges_real(filter[attribute].str()));
 }

diff --git a/src/utils/config_filters.hpp b/src/utils/config_filters.hpp
index 878042a089e..a01a97f4db3 100644
--- a/src/utils/config_filters.hpp
+++ b/src/utils/config_filters.hpp
@@ -40,8 +40,8 @@ bool bool_matches_if_present(const config& filter, const config& cfg, const std:
  *
  * Always returns true if the filter puts no restriction on the value of @a cfg[@a attribute].
  */
-bool double_matches_if_present(const config& filter, const config& cfg, const std::string& attribute, std::optional<double> def = NULL);
-bool int_matches_if_present(const config& filter, const config& cfg, const std::string& attribute, std::optional<int> def = NULL);
+bool double_matches_if_present(const config& filter, const config& cfg, const std::string& attribute, double def = 0);
+bool int_matches_if_present(const config& filter, const config& cfg, const std::string& attribute, int def = 0);

 /**
  * Restricts filters to only looking for values that are zero or more.
@@ -62,7 +62,7 @@ bool unsigned_matches_if_present(const config& filter, const config& cfg, const
  * The function is named "negative" in case we later want to add a "reciprocal" for the "multiply"/"divide" pair.
  */
 bool int_matches_if_present_or_negative(
-       const config& filter, const config& cfg, const std::string& attribute, const std::string& opposite, std::optional<int> def = NULL);
+       const config& filter, const config& cfg, const std::string& attribute, const std::string& opposite, int def = 0);

 bool string_matches_if_present(
        const config& filter, const config& cfg, const std::string& attribute, const std::string& def);

Sorry if I'm wrong. And let me know if I am, plz. @stevecotton

@Pentarctagon
Copy link
Member

Was this meant to be closed?

@iFoundSilentHouse
Copy link
Author

NO SORRY

@iFoundSilentHouse
Copy link
Author

Accidentally pushed empty commit

@iFoundSilentHouse
Copy link
Author

Now build works and bug should no longer exist

@stevecotton
Copy link
Contributor

@newfrenchy83 would you review this PR please?

@newfrenchy83
Copy link
Contributor

@newfrenchy83 would you review this PR please?

Not that I'm happy to revert to the optionality of the default, but if it actually triggers a compilation problem then I'll accept it.

@newfrenchy83
Copy link
Contributor

@iFoundSilentHouse did you try compiling with std::nullopt instead of NULL, rather than forgoing std::optional?

@iFoundSilentHouse
Copy link
Author

@iFoundSilentHouse did you try compiling with std::nullopt instead of NULL, rather than forgoing std::optional?

No. I didn't.

@stevecotton
Copy link
Contributor

@newfrenchy83 if NULL was always zero instead of being equivalent to nullopt, then I'm wondering if the optional things need to be optional at all.

@newfrenchy83
Copy link
Contributor

@newfrenchy83 if NULL was always zero instead of being equivalent to nullopt, then I'm wondering if the optional things need to be optional at all.

I had confused NULL with std::nullopt, hence the confusion, for me it is not a question of using NULLL as an equivalent of 0

@iFoundSilentHouse
Copy link
Author

Closing this PR in favor of more accurate #8649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Building Build-time issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants