Skip to content

general compile error and warning fixes #492

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

general compile error and warning fixes #492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/json/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@
#define JSON_USE_EXCEPTION 1
#endif

/// If defined, indicates that the source file is amalgated
/// If defined, indicates that the source file is amalgamated
/// to prevent private header inclusion.
/// Remarks: it is automatically defined in the generated amalgated header.
/// Remarks: it is automatically defined in the generated amalgamated header.
// #define JSON_IS_AMALGAMATION

#ifdef JSON_IN_CPPTL
Expand Down
2 changes: 2 additions & 0 deletions src/lib_json/json_reader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#include <memory>
#include <set>
#include <limits>
#include <stdio.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Minimally, wouldn't these have to be cstring and cstdio for strict compatibility with modern compilers?

Adding headers is fine. But could you tell us where this was failing to compile?

Copy link
Author

Choose a reason for hiding this comment

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

Beats me. I dont use modern compilers or know anything about them. This was failing on visual c++ 2010.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we can continue to support VS2010 for the master branch. 0.y.z will work fine.

But could you try #include <cstdio> and #include <cstring> in VS2010, and if it works, push that to this branch to update this pull-request?

Copy link
Author

Choose a reason for hiding this comment

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

Ahh sorry I was mistaken. It wasn't VS2010, it was a ghetto version of ARMCC I have to use. Each of the files I added stdio.h or string.h to already had <cstring> and<cstdio> in it, and it wasn't sufficient--so your test is already done.

I noticed this snippet in one place (and snippets like it in many other places)

#if defined(__QNXNTO__)
#define sscanf std::sscanf
#endif`

This is representative of another solution to the actual problem I had (symbols like sscanf being undeclared, being in the std:: namespace instead). So, it seems you've dealt with this problem before.

Since I don't know anything about compilers from the present or the future (it's all the same to a guy stuck in the past) all I can do is naively google up things like http://en.cppreference.com/w/cpp/io/c/sscanf which seems to say those functions belong in std:: namespace indeed and assume your great modern compilers which support them without the std:: qualifier are leading you astray

I'll let you think on this a while before I do any reverting. I can't competently suggest anything, but can incompetently suggest you sprinkle the code with std:: liberally if you want to be standards-compliant with c++29 or whatever you're targeting. For instance, I fixed json_writer.cpp in a different way by adding six std::

Copy link
Contributor

Choose a reason for hiding this comment

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

your great modern compilers which support them without the std:: qualifier are leading you astray

Some older compilers have problems with std:: sometimes, and sscanf in particular can be available in weird ways. We'll be standards compliant in 2.0, but for now it's helpful to some users if we avoid being too strict.

I'll let you think on this a while before I do any reverting.

Don't revert. But can you wrap the new include lines in and ifdef for your compiler? IIRC, some modern compilers actually do not include the old headers, or consider them a compiler warning (which becomes an error with -Werror).

For instance, I fixed json_writer.cpp in a different way by adding six std::

Could you show us where? We can't add that on 0.y.z, but on master I think that would be fine.

#include <string.h>

#if defined(_MSC_VER)
#if !defined(WINCE) && defined(__STDC_SECURE_LIB__) && _MSC_VER >= 1500 // VC++ 9.0 and above
Expand Down
3 changes: 3 additions & 0 deletions src/lib_json/json_value.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#endif
#include <cstddef> // size_t
#include <algorithm> // min()
#include <string.h>

#define JSON_ASSERT_UNREACHABLE assert(false)

Expand Down Expand Up @@ -190,6 +191,7 @@ static inline void releaseStringValue(char* value, unsigned) {

namespace Json {

#if JSON_USE_EXCEPTION
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the need for this guard. As long as we don't throw an exception, who cares if exceptions are define? Also, I think it's still declared, right?

Copy link
Author

Choose a reason for hiding this comment

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

That code contains 'throw'. When exceptions are disabled in the compiler, throw is unavailable.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we simply guard the throw? We could also abort() on the next line. JsonCpp will not call these throw* functions when JSON_USE_EXCEPTIONS is false anyway.

Exception::Exception(JSONCPP_STRING const& msg)
: msg_(msg)
{}
Expand All @@ -213,6 +215,7 @@ JSONCPP_NORETURN void throwLogicError(JSONCPP_STRING const& msg)
{
throw LogicError(msg);
}
#endif

// //////////////////////////////////////////////////////////////////
// //////////////////////////////////////////////////////////////////
Expand Down
6 changes: 5 additions & 1 deletion src/lib_json/json_writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#include <cassert>
#include <cstring>
#include <cstdio>
#include <stdio.h>
#include <string.h>

#if defined(_MSC_VER) && _MSC_VER >= 1200 && _MSC_VER < 1800 // Between VC++ 6.0 and VC++ 11.0
#include <float.h>
Expand Down Expand Up @@ -42,9 +44,11 @@
#else
#include <cmath>
#if !(defined(__QNXNTO__)) // QNX already defines isfinite
#ifndef isfinite
Copy link
Contributor

Choose a reason for hiding this comment

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

Good!

#define isfinite std::isfinite
#endif
#endif
#endif

#if defined(_MSC_VER)
#if !defined(WINCE) && defined(__STDC_SECURE_LIB__) && _MSC_VER >= 1500 // VC++ 9.0 and above
Expand Down Expand Up @@ -143,7 +147,7 @@ JSONCPP_STRING valueToString(double value, bool useSpecialFloats, unsigned int p
int len = -1;

char formatString[6];
sprintf(formatString, "%%.%dg", precision);
sprintf(formatString, "%%.%ug", precision);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good.


// Print into the buffer. We need not request the alternative representation
// that always has a decimal point because JSON doesn't distingish the
Expand Down