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

no member named 'thousands_sep' in 'lconv' #631

Closed
rcdailey opened this issue Jun 21, 2017 · 27 comments
Closed

no member named 'thousands_sep' in 'lconv' #631

rcdailey opened this issue Jun 21, 2017 · 27 comments
Labels
platform: android related to Android NDK solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@rcdailey
Copy link

json.hpp:8337:46: error: no member named 'thousands_sep' in 'lconv'

            const char thousands_sep = !loc->thousands_sep ? '\0'

                                        ~~~  ^

Getting this error using NDK r14b with armeabi-v7a architecture, API 15, and clang + shared gnu STL. Works fine on MSVC 14 though. How can we get Android working?

@rcdailey
Copy link
Author

Looks like with most things involving the NDK, this symbol (thousands_sep) was introduced in API 21. So for API 15, it is not available. I think the code should handle this missing symbol with some sort of workaround or alternative solution.

Starting with NDK 14, a central "sysroot" is available that may solve this issue but that's still not ready for prime time.

@nlohmann nlohmann added the platform: android related to Android NDK label Jun 22, 2017
@nlohmann
Copy link
Owner

The problem seems that this specific version has no proper locale header, but only a stub. It seems others have had this problem as well.

This patch or this patch seem to be solutions.

I see what I can do about this.

@rcdailey
Copy link
Author

Do you plan on implementing a workaround soon? I will not be able to integrate your library into our work code base until this is addressed. Happy to wait and test any solution for you. Just let me know.

@nlohmann
Copy link
Owner

No, I'm sorry this has no priority right now.

You may replace function

        /// return the locale-dependent decimal point
        static char get_decimal_point() noexcept
        {
            const auto loc = localeconv();
            assert(loc != nullptr);
            return (loc->decimal_point == nullptr) ? '.' : loc->decimal_point[0];
        }

with

        /// return the locale-dependent decimal point
        static char get_decimal_point() noexcept
        {
            return '.';
        }

to avoid using the broken localeconv in your standard library.

@rcdailey
Copy link
Author

How will that one fix the thousands_sep issue?

@nlohmann
Copy link
Owner

Oh, sorry, wrong function. Change

        serializer(output_adapter_t<char> s, const char ichar)
            : o(s), loc(std::localeconv()),
              thousands_sep(!loc->thousands_sep ? '\0' : loc->thousands_sep[0]),
              decimal_point(!loc->decimal_point ? '\0' : loc->decimal_point[0]),
              indent_char(ichar), indent_string(512, indent_char)
        {}

to

        serializer(output_adapter_t<char> s, const char ichar)
            : o(s), loc(std::localeconv()),
              thousands_sep('\0'),
              decimal_point('\0'),
              indent_char(ichar), indent_string(512, indent_char)
        {}

@gregmarr
Copy link
Contributor

This is in the develop branch. There is similar code in master, but it's not encapsulated in functions like this.

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Jun 22, 2017
@nlohmann
Copy link
Owner

Please let me know if you can compile the code with this fix. If you execute the test suite, some locale-specific tests should fail, but apart from this, I wonder if the library works.

@rcdailey
Copy link
Author

I wonder if a better solution here would be for me to implement my own locale, because you're not really doing anything wrong. It's Android's NDK that's busted.

@gregmarr
Copy link
Contributor

That's sort of what the patches that @nlohmann referenced in his first comment above do. They add a wrapper that returns the data if it's there, and dummy data otherwise. The most recent solution would be one that you'd have to apply on your end, but I assume those patches could be applied here as a PR if you test them out in your own fork first. I can't speak for @nlohmann, I'm just a user.

@nlohmann
Copy link
Owner

I think it would be best if the users of a broken NDK would provide a proper locale implementation and I could leave my library unchanged.

@gregmarr
Copy link
Contributor

That would be nice, but I'm not sure if it's possible to replace the locale implementation without changes to the library as in those patches that you referenced, leaving local patching of the library or not supporting broken NDKs. It wouldn't be the first time there were NDKs that this didn't support.

@rcdailey
Copy link
Author

I've never replaced system locale before (and it looks like this is done via C API and not normal STL locale). I'd assume I could just provide a different (but interface-same) implementation of that struct.

@gregmarr
Copy link
Contributor

The problem is that the interface of that struct is empty. It's a compile-time failure, not a runtime failure, so you'd have to replace the definition of the struct in the NDK.

@nlohmann
Copy link
Owner

That's a shame. Then maybe, one of the linked patches could help. I currently think, the first one could used with a static_if could be the change with the fewest changes (PRs welcome!)

In any case, I would be happy to know whether the quick fix above makes the library compile.

@rcdailey
Copy link
Author

At the moment I'm using the single header from the latest tagged release. If I use develop, do you guys have the single header already updated at the tip?

@gregmarr
Copy link
Contributor

develop only has the single header. It's no longer using the re2c file.

@rcdailey
Copy link
Author

rcdailey commented Jun 23, 2017

Looks like your 2 changes are working just fine. Since I cannot simply implement my own locale as mentioned before, it seems that some changes will be required to your json library to support Android NDK prior to API 21.

What would be the best way to do this? Maybe a preprocessor switch: DISABLE_SYSTEM_LOCALE or something? What other ways could the user specify the bits of locale information you need?

I'm happy to contribute a patch, so just let me know design-wise how you'd like to approach this problem.

@sasmaster
Copy link

Yep,I have stumbled into this one as well.The problem is that Android SDK 19 has very large user segment.Therefore this issue is quite critical.Thanks.

@sasmaster
Copy link

sasmaster commented Jun 26, 2017

The above patch doesn't help because there is also this line:

  const auto loc = localeconv();
                assert(loc != nullptr);
                const char decimal_point_char = (loc->decimal_point == nullptr) ? '.' : loc->decimal_point[0];

@sasmaster
Copy link

Can I safely change it to this:

const char decimal_point_char = "."

?

@nlohmann
Copy link
Owner

Yes

@sasmaster
Copy link

Yeah,that helps,thanks! Any plan to put this patch into official release?

@nlohmann
Copy link
Owner

No, because this patch removes a feature (locale-independence). We need to find a way to properly detect the broken STL of that NDK.

@nlohmann
Copy link
Owner

nlohmann commented Jul 8, 2017

I have problems reproducing the issue, see #638 (comment). Help is greatly appreciated!

@nlohmann
Copy link
Owner

Does the fix for #638 work here?

@sasmaster
Copy link

Yes it does

micasill pushed a commit to microsoft/cpp_client_telemetry that referenced this issue Aug 15, 2019
… lconv not being available on some versions of the Android SDK.
micasill pushed a commit to microsoft/cpp_client_telemetry that referenced this issue Aug 15, 2019
Apply Nlohmann's fix in nlohmann/json#631 to address localeconv() and lconv not being available on some versions of the Android SDK.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: android related to Android NDK solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

4 participants