C4996 error and warning with Visual Studio

[prsyahmi]

Using latest commit, C4996 errors and warnings spew on every string manipulation function calls:

• std::strcpy (Warning)
• std::strcat (Warning)
• std::copy (Error - compile fail) on non-iterator/pointer destination

Error C4996 'std::copy::_Unchecked_iterators::_Deprecate': Call to 'std::copy' with parameters that may be unsafe - this call relies on the caller to check that the passed values are correct. To disable this warning, use -D_SCL_SECURE_NO_WARNINGS. See documentation on how to use Visual C++ 'Checked Iterators'

changing std::copy(m_start, m_end, buf.data()); to std::copy(m_start, m_end, buf.begin()); fix this problem.

---

warning C4996: 'strcat': This function or variable may be unsafe. Consider using strcat_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS.

I think turning off the warning like proposed on #453 before might be enough but probably not a clean solution.

[nlohmann]

Thanks for reporting! I had some headaches taking over the code with strcat/strlen, but it seemed more elegant compared to a manual search for the \0 byte. However, this manual search can be done in a for loop in this setting, so it shall be safe to replace the array<char, N> by a std::vector<char> or std::string with respective reservation of N bytes.

[prsyahmi]

Since dealing with string here, In my opinion the std::string with reservation of N bytes approach is a better choice. There will be no need to call strcat/strcpy or do it manually since we got + operator and = operator. The strlen you mentioned isn't affected and still can be used, but switching to .length() is recommended.

There is only one usage of strlen on array data I found in latest commit

const auto data_end = m_buf.begin() + strlen(m_buf.data());

const bool value_is_int_like =
      std::none_of(m_buf.begin(), data_end, [](const char c)
      {
           return (c == '.' or c == 'e' or c == 'E');
      });

which then can be switched to .cbegin(), .cend() instead.

[TurpentineDistillery]

The rationale for using std::array<char, N> instead of a std::string in this context was to avoid heap allocations, which is the performance bottleneck.

[prsyahmi]

Well both std::string and std::vector does heap allocation. So for performance critical, I think ignoring the warning worth it, since we control the input data ".", "-0.0" and "0.0".

[prsyahmi]

Another solution for the warnings

#if defined(_MSC_VER)
#define JSON_STRCPY strcpy_s
#define JSON_STRCAT strcat_s
#define JSON_STROUT(out, buflen) out, buflen
#else
#define JSON_STRCPY std::strcpy
#define JSON_STRCAT std::strcat
#define JSON_STROUT(out, buflen) out
#endif

Then call like this JSON_STRCAT(JSON_STROUT(m_buf.data(), m_buf.size()), ".0");

and finally

#undef JSON_STRCAT
#undef JSON_STRCPY
#undef JSON_STROUT

What do you think?

[nlohmann]

I did some refactoring to avoid the unsafe calls, see 83a9c60. I combined a call to strlen and a std::none_of to a single for loop and unrolled stdcpy/ strcat to a sequence of assignments. What do you think?

[nlohmann]

@prsyahmi Could you please try with the most recent version?

[prsyahmi]

The code looks good and it compiles fine here, no more warning and error 👍 I guess this can be closed now. Thanks!