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

* PVS Studio fix #1217

Conversation

pavel-pimenov
Copy link
Contributor

Copy link
Contributor

@BillyDonahue BillyDonahue left a comment

Choose a reason for hiding this comment

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

looks like a lot of unnecessary changes, largely to c++14. Is there actually a "fix" in here?

Copy link
Contributor

@BillyDonahue BillyDonahue left a comment

Choose a reason for hiding this comment

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

Thanks for the persistence.
I think you're on the right track with the make_unique but we have to stay C++11 compatible, so we should abstract it internally.

@@ -840,6 +840,7 @@ bool Value::isConvertibleTo(ValueType other) const {
(type() == realValue && InRange(value_.real_, 0, maxUInt)) ||
type() == booleanValue || type() == nullValue;
case realValue:
return isNumeric() || type() == booleanValue || type() == nullValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also use the [[fallthrough]] attribute here to document the intent instead of duplicating code, and that would shut PVS Studio up.
https://en.cppreference.com/w/cpp/language/attributes/fallthrough

I think the duplication here is probably an improvement though. It's almost coincidental that convertibility to realValue and convertibility to booleanValue are the same condition.

@@ -1396,7 +1397,7 @@ String Value::Comments::get(CommentPlacement slot) const {

void Value::Comments::set(CommentPlacement slot, String comment) {
if (!ptr_) {
ptr_ = std::make_unique<Array>();
ptr_ = std::unique_ptr<Array>(new Array());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should make a version of make_unique in this file with internal linkage.

It can forward to std::make_unique if __cplusplus indicates that we're on C++14. Then we won't have to deal with the make_unique suggestion again. People are going to keep running jsoncpp through linters and sending PRs if we keep the C++11 raw new unique_ptr creation idiom in place. :)

if (cs_str == "All") {
cs = CommentStyle::All;
} else if (cs_str == "None") {
if (cs_str == "None") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong. Am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CommentStyle::Enum cs = CommentStyle::All;

@@ -408,7 +408,7 @@ Json::String ToJsonString(const char* toConvert) {
return Json::String(toConvert);
}

Json::String ToJsonString(Json::String in) { return in; }
Json::String ToJsonString(const Json::String& in) { return in; }
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to modify the header:

  jsoncpp_static.vcxproj -> C:\projects\jsoncpp\lib\Release\jsoncpp_static.lib
main.obj : error LNK2019: unresolved external symbol "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl JsonTest::ToJsonString(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >)" (?ToJsonString@JsonTest@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V23@@Z) referenced in function "public: virtual void __thiscall TestCharReaderAllowSingleQuotesTestissue182::runTestCase(void)" (?runTestCase@TestCharReaderAllowSingleQuotesTestissue182@@UAEXXZ) [C:\projects\jsoncpp\src\test_lib_json\jsoncpp_test.vcxproj]

@baylesj
Copy link
Contributor

baylesj commented Sep 12, 2024

Closing this due to inactivity. Feel free to reopen if you are able to address feedback.

@baylesj baylesj closed this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants