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

Compiling issues with libc 2.12 #1483

Closed
naszta opened this issue Feb 14, 2019 · 11 comments
Closed

Compiling issues with libc 2.12 #1483

naszta opened this issue Feb 14, 2019 · 11 comments
Assignees
Labels
release item: 🔨 further change solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@naszta
Copy link
Contributor

naszta commented Feb 14, 2019

Possible fix:
#if !defined(NDEBUG) && !defined(__STDC_LIMIT_MACROS)
#define __STDC_LIMIT_MACROS
#endif
(may be with #pragma push_macro and #pragma pop_macro)
to
https://github.com/nlohmann/json/blob/develop/include/nlohmann/detail/conversions/to_chars.hpp#L2

@nickaein
Copy link
Contributor

nickaein commented Feb 15, 2019

As have stated in your bugzilla link, this is more like a libc bug that has been fixed in newer versions. If switching to newer versions are off the table, can you use the workaround your proposed in your application?

Adding this workaround to the library code could be problematic. Defining __STDC_LIMIT_MACROS along with all macros with double underscores are reserved in C++ standard. Strictly speaking, defining a macro with double underscores is undefined behavior:

"Reserved" here means that the standard library headers #define or declare such identifiers for their internal needs, the compiler may predefine non-standard identifiers of that kind, and that name mangling algorithm may assume that some of these identifiers are not in use. If the programmer uses such identifiers, the behavior is undefined.

However, defining such macro inside an application that its code isn't going to spread in multiple place and referenced in various people could be probably safe and can be swept under the rug.

@garethsb
Copy link
Contributor

A user of my library hit this with glibc 2.17 also. So far as I can tell, defining __STDC_LIMIT_MACROS is harmless.

More background here:
https://sourceware.org/ml/libc-alpha/2013-04/msg00598.html

@naszta
Copy link
Contributor Author

naszta commented Feb 19, 2019

I thought that adding a macro to the source would keep the issue locally. Older libc versions are widely used so it would be a big help for many developers. BTW: the best solution would be just simply use std::numeric_limits<type>::max() against macros.

@nickaein
Copy link
Contributor

std::numeric_limits<T>::max() seems cleaner than macros. I've done some digging and it seems that it is defined for the basic arithmetic types and the standard has left the specialization of types like uint64_t optional and up to implementer. So to be pedantic, std::numeric_limits<uint64_t>::max() is not guaranteed to be available on a conforming C++11 compiler while INT64_MAX is.

In practice, it is very unlikely that an library implementer left these specializations out though.

@naszta
Copy link
Contributor Author

naszta commented Feb 20, 2019

We know the supported compilers. We could have issues with macros on the supported compilers while we know that numeric limits work nicely. I would go for numeric limits.

@nlohmann
Copy link
Owner

Any proposal how to proceed here?

@naszta
Copy link
Contributor Author

naszta commented Mar 11, 2019

I'm happy to create the change (from macro to numerical_limits). I wouldn't create, if it won't be merged. So are you okay with that solution?

@nlohmann
Copy link
Owner

Yes, please change to numeric_limits.

@naszta
Copy link
Contributor Author

naszta commented Mar 11, 2019

I create it till Friday.

naszta added a commit to naszta/json that referenced this issue Mar 13, 2019
@naszta
Copy link
Contributor Author

naszta commented Mar 13, 2019

#1514 is opened.

@nlohmann
Copy link
Owner

Closed by merging #1514.

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation release item: 🔨 further change and removed state: waiting for PR labels Mar 15, 2019
@nlohmann nlohmann self-assigned this Mar 15, 2019
@nlohmann nlohmann added this to the Release 3.6.0 milestone Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release item: 🔨 further change 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