🔧 Fix up a few more effc++ items #858

mattismyname · 2017-12-05T19:07:36Z

test suite has these two failures, but the same tests fail even before this change:

21/70 Test #62: test-regression_all .................***Failed 12.56 sec
29/70 Test #61: test-regression_default .............***Failed 12.91 sec

nlohmann · 2017-12-05T19:21:01Z

The tests should succeed... Can you make sure you on the latest develop version?

theodelrieu · 2017-12-05T19:23:00Z

What are the errors?

mattismyname · 2017-12-05T19:30:54Z

Ok, my bad. The failures were with gcc 4.9.3. When run under 5.4.0-6ubuntu1~16.04.5 everything passes.

nlohmann · 2017-12-05T19:31:52Z

Hm... GCC 4.9 should work as well. Could you post the errors?

nlohmann · 2017-12-05T19:32:13Z

And which compiler created the -Weffc++ warnings?

mattismyname · 2017-12-05T19:44:04Z

Original effc++ warnings were from icc (ICC) 11.0 20081105. I'll dig them up and post them here.

Sorry, never used ctest -- how can I get it to print the specific errors? I just get very general message currently:

$ ctest -R test-regression_default
Test project /nfs/orto/home/mkgumbel/clt_dev02/json/build
    Start 61: test-regression_default
1/1 Test #61: test-regression_default ..........***Failed    6.87 sec

0% tests passed, 1 tests failed out of 1

Label Time Summary:
default    =   6.87 sec (1 test)

Total Test time (real) =   6.94 sec

The following tests FAILED:
         61 - test-regression_default (Failed)
Errors while running CTest

thanks

nlohmann · 2017-12-05T19:45:02Z

You can use --output-on-failure.

coveralls · 2017-12-06T05:12:21Z

Coverage remained the same at 100.0% when pulling 3fee464 on mattismyname:develop into 0e3a0b7 on nlohmann:develop.

nlohmann · 2017-12-06T19:59:44Z

src/json.hpp

@@ -6101,7 +6101,9 @@ class serializer
    @param[in] ichar  indentation character to use
    */
    serializer(output_adapter_t<char> s, const char ichar)
-        : o(std::move(s)), loc(std::localeconv()),
+        : o(std::move(s)),
+          number_buffer(),


What is the warning that led to adding number_buffer() here? The member is initialized in the definition:

std::array<char, 64> number_buffer{{}};

It was bad icc check -- I think this change is not really necessary.

Thanks for checking back. Could you revert this change please?

nlohmann · 2017-12-06T20:02:00Z

src/json.hpp

@@ -8011,6 +8013,7 @@ class basic_json
        json_value(number_float_t v) noexcept : number_float(v) {}
        /// constructor for empty values of a given type
        json_value(value_t t)
+            : object(nullptr)


If this is required, then maybe we can remove the lines

object = nullptr; // silence warning, see #821

below which have been added when discussing #821.

Related: line 8073 is now unreachable due to the case added at line 8062. That line should be moved into the new case if it's still desirable, or removed altogether along with the surrounding if().

That case was always unreachable and was only added to get the hash 961c151d2e87f2686a955a9be24d316f1362bf21 into binaries using the library.

Interesting. Perhaps a comment is in order then.

I think this can be removed -- it was bad icc check.

Thanks. Could you remove the mentioned line above please?

nlohmann · 2017-12-08T06:57:23Z

@mattismyname Could you comment on #858 (review) and #858 (review)?

nlohmann · 2017-12-12T20:01:26Z

@mattismyname Could you please comment on #858 (review) and #858 (review)? I would like to have this PR merged for the upcoming release.

mattismyname · 2017-12-12T20:20:12Z

Sorry for slowness

nlohmann · 2017-12-12T20:22:51Z

src/json.hpp

@@ -6101,7 +6101,9 @@ class serializer
    @param[in] ichar  indentation character to use
    */
    serializer(output_adapter_t<char> s, const char ichar)
-        : o(std::move(s)), loc(std::localeconv()),
+        : o(std::move(s)),
+          number_buffer(),


Thanks for checking back. Could you revert this change please?

nlohmann · 2017-12-12T20:23:23Z

src/json.hpp

@@ -8011,6 +8013,7 @@ class basic_json
        json_value(number_float_t v) noexcept : number_float(v) {}
        /// constructor for empty values of a given type
        json_value(value_t t)
+            : object(nullptr)


Thanks. Could you remove the mentioned line above please?

mattismyname · 2017-12-24T18:01:24Z

I've pushed the requested fixes to mattismyname/json develop branch.

coveralls · 2017-12-24T20:18:15Z

Coverage remained the same at 100.0% when pulling 72bff90 on mattismyname:develop into 3113a52 on nlohmann:develop.

nlohmann

Looks good to me.

nlohmann · 2017-12-28T11:13:27Z

Thanks a lot!

Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::Range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of compiler suggestions, and no further explanation is discernible from the PR discussion (nlohmann#858). This commit partially reverts f28fc22, removing all added const qualifiers. Fixes nlohmann#3331.

Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of compiler suggestions, and no further explanation is discernible from the PR discussion (nlohmann#858). This commit partially reverts f28fc22, removing all added const qualifiers. Fixes nlohmann#3331.

Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of ICC compiler suggestions, and no further explanation is discernible from the PR discussion (nlohmann#858). Further testing revealed, that clang-tidy also suggests adding const to prevent "accidental mutation of a temporary object". As an alternative, this commit partially reverts f28fc22, removing all added const qualifiers from return types and adds lvalue reference qualifiers to the operator member functions instead. Unit tests ensure the operators remain equality-preserving and accidental mutation of temporaries following post-(inc-/dec-)rement is prohibited. Fixes nlohmann#3331.

Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of ICC compiler suggestions, and no further explanation is discernible from the PR discussion (#858). Further testing revealed, that clang-tidy also suggests adding const to prevent "accidental mutation of a temporary object". As an alternative, this commit partially reverts f28fc22, removing all added const qualifiers from return types and adds lvalue reference qualifiers to the operator member functions instead. Unit tests ensure the operators remain equality-preserving and accidental mutation of temporaries following post-(inc-/dec-)rement is prohibited. Fixes #3331.

nlohmann reviewed Dec 6, 2017

View reviewed changes

nlohmann requested changes Dec 12, 2017

View reviewed changes

mattismyname force-pushed the develop branch from 3fee464 to bbe61d7 Compare December 24, 2017 17:56

🔧 Fix up a few more effc++ items

72bff90

mattismyname force-pushed the develop branch from bbe61d7 to 72bff90 Compare December 24, 2017 18:04

nlohmann approved these changes Dec 28, 2017

View reviewed changes

nlohmann self-assigned this Dec 28, 2017

nlohmann added this to the Release 3.0.1 milestone Dec 28, 2017

nlohmann merged commit f28fc22 into nlohmann:develop Dec 28, 2017

gregmarr mentioned this pull request Feb 15, 2022

Iterator doesn't satisfy std::incrementable because post-increment may change constness #3331

Closed

3 tasks

falbrechtskirchinger mentioned this pull request Feb 15, 2022

Make iterator operator++/--(int) equality-preserving #3332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔧 Fix up a few more effc++ items #858

🔧 Fix up a few more effc++ items #858

mattismyname commented Dec 5, 2017

nlohmann commented Dec 5, 2017

theodelrieu commented Dec 5, 2017

mattismyname commented Dec 5, 2017

nlohmann commented Dec 5, 2017

nlohmann commented Dec 5, 2017

mattismyname commented Dec 5, 2017

nlohmann commented Dec 5, 2017

coveralls commented Dec 6, 2017

nlohmann Dec 6, 2017

mattismyname Dec 12, 2017

nlohmann Dec 12, 2017

nlohmann Dec 6, 2017

gregmarr Dec 6, 2017 •

edited

Loading

nlohmann Dec 6, 2017

gregmarr Dec 6, 2017 •

edited

Loading

mattismyname Dec 12, 2017

nlohmann Dec 12, 2017

nlohmann commented Dec 8, 2017

nlohmann commented Dec 12, 2017

mattismyname commented Dec 12, 2017

nlohmann Dec 12, 2017

nlohmann Dec 12, 2017

mattismyname commented Dec 24, 2017

coveralls commented Dec 24, 2017

nlohmann left a comment

nlohmann commented Dec 28, 2017

🔧 Fix up a few more effc++ items #858

🔧 Fix up a few more effc++ items #858

Conversation

mattismyname commented Dec 5, 2017

nlohmann commented Dec 5, 2017

theodelrieu commented Dec 5, 2017

mattismyname commented Dec 5, 2017

nlohmann commented Dec 5, 2017

nlohmann commented Dec 5, 2017

mattismyname commented Dec 5, 2017

nlohmann commented Dec 5, 2017

coveralls commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmarr Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmarr Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlohmann commented Dec 8, 2017

nlohmann commented Dec 12, 2017

mattismyname commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattismyname commented Dec 24, 2017

coveralls commented Dec 24, 2017

nlohmann left a comment

Choose a reason for hiding this comment

nlohmann commented Dec 28, 2017

gregmarr Dec 6, 2017 •

edited

Loading

gregmarr Dec 6, 2017 •

edited

Loading