include/json: check MSVC before adding pragma warning directive #1212
Conversation
There was a problem hiding this comment.
Instead of modifying every instance of #if defined(JSONCPP_DISABLE_DLL_INTERFACE_WARNING),
we can just go find where JSONCPP_DISABLE_DLL_INTERFACE_WARNING is defined and force it to be false unless _MSC_VER is defined. The JSONCPP_.... macros are supposed to be stand-alone and not require compound conditions like this.
include/json/writer.h
Outdated
| #if defined(JSONCPP_DISABLE_DLL_INTERFACE_WARNING) && defined(_MSC_VER) | ||
| #pragma warning(pop) | ||
| #endif // if defined(JSONCPP_DISABLE_DLL_INTERFACE_WARNING) | ||
| #endif // if defined(JSONCPP_DISABLE_DLL_INTERFACE_WARNING) && defined(_MSC_VER) |
There was a problem hiding this comment.
Let's just delete the comments on these #endif statements if the block is small and leafy like this. It's lame to keep them up to date as the #if condition evolves.
There was a problem hiding this comment.
I just looked at json/src/config.h and we already do what I'm requesting.
The JSONCPP_DISABLE_DLL_INTERFACE_WARNING is only defined if _MSC_VER is defined.
This is by design. I don't think your PR will improve anything. Do you have a motivating case like a build failure you can provide?
|
I have change the code and add a little explanation. |
* File changes: include/json/{config.h, writer.h}
* Summary: previously JSONCPP_DISABLE_DLL_INTERFACE_WARNING is defined
when _MSC_VER or __MINGW32__ defined. So, when compiled with GCC or
Clang using MinGW toolchain those #pragma warnings are also enabled.
This change enables those #pragma warnings only when MSVC is used
i.e. only when _MSC_VER is defined.
| #endif // if defined(_MSC_VER) | ||
| #endif // ifdef JSON_DLL_BUILD | ||
|
|
||
| // Disable MSVC specific compiler warnings |
There was a problem hiding this comment.
This macro used to only get defined when JSON_DLL_BUILD is defined.
I think it should stay that way, as it's clearly only relevant to DLL builds. Move it back up into the preceding #if block.
There was a problem hiding this comment.
But it was defined in both JSON_DLL_BUILD and JSON_DLL sections. What's the issue with current change?
There was a problem hiding this comment.
Sorry, I misread and thought JSON_DLL was nested inside JSON_DLL_BUILD.
So looking at the if/else structure here, we have 3 relevant modes. JSON_DLL, JSON_DLL_BUILD, and everything else.
I think JSONCPP_DISABLE_DLL_INTERFACE_WARNING should only be engaged when we're making a DLL target AND we're using a MSFT compiler. I think we should only disable the warnings when we expect to generate them and need to silence them. We don't need this warning silenced if JSON_API doesn't expand into a dll-related declspec. If we're not building with DLL interface declspecs, we are silencing a warning unnecessarily and we don't need to do that. So I think we should be extremely conservative in turning off warnings. So:
#if defined(_MSC_VER) && (defined (JSON_DLL) || defined(JSON_DLL_BUILD))
#define JSONCPP_DISABLE_DLL_INTERFACE_WARNING
#endif
?
There was a problem hiding this comment.
But JSONCPP_DISABLE_DLL_INTERFACE_WARNING is defined in include/json files. So, if a project includes those header files MSVC compiler shows warnings due to jsoncpp.
There was a problem hiding this comment.
Looping back around to this. I think the confusion is that the warnings you are seeing are NOT DLL_INTERFACE_WARNINGs. This should really be renamed to:
JSONCPP_DISABLE_MSVC_WARNINGS
|
It's still not clear to me what the actual error that is happening in MSVC, and how this fixes it. @Biswa96 can you show the compiler output? |
|
The error is not happening with MSVC, it's with GCC and/or Clang. First read the commit message. The In file included from ../include/json/reader.h:11,
from ../src/lib_json/json_reader.cpp:10:
../include/json/value.h:52: warning: ignoring '#pragma warning ' [-Wunknown-pragmas]
52 | #pragma warning(push)
|
../include/json/value.h:53: warning: ignoring '#pragma warning ' [-Wunknown-pragmas]
53 | #pragma warning(disable : 4251)
|
../include/json/value.h:932: warning: ignoring '#pragma warning ' [-Wunknown-pragmas]
932 | #pragma warning(pop)
| |
You said here: #1212 (comment)
What MSVC compiler warnings are we talking about in THAT case? |
|
The warnings in |
4 participants
No description provided.