Code modernization (empty, for-range, auto) #787

Nekto89 · 2018-06-04T20:53:16Z

Using empty() increases readability.

Use auto when declaring iterators Use auto when initializing with a cast to avoid duplicating the type name

BillyGoto · 2018-06-04T22:04:20Z

In the library code, yes. But Please consider the assertions carefully. When they fail, they should be able to print some information about what was expected and what was received, and a size() or a string value is better diagnostic information than just whether something is not empty().

On Mon, Jun 4, 2018 at 4:53 PM Marian Klymov ***@***.***> wrote: Using empty() increases readability. ------------------------------ You can view, comment on, or merge this pull request online at: #787 Commit Summary - Use empty() for emptiness checks. File Changes - *M* src/lib_json/json_reader.cpp <https://github.com/open-source-parsers/jsoncpp/pull/787/files#diff-0> (4) - *M* src/lib_json/json_value.cpp <https://github.com/open-source-parsers/jsoncpp/pull/787/files#diff-1> (4) - *M* src/test_lib_json/main.cpp <https://github.com/open-source-parsers/jsoncpp/pull/787/files#diff-2> (75) Patch Links: - https://github.com/open-source-parsers/jsoncpp/pull/787.patch - https://github.com/open-source-parsers/jsoncpp/pull/787.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#787>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHFzPLHmt9i9wipFE5zk8inUdBTMgDzLks5t5Z5CgaJpZM4UZw9X> .

-- ǝnɥɐuop ʎllıq

BillyDonahue · 2018-06-05T14:32:43Z

src/test_lib_json/jsontest.cpp

@@ -436,4 +436,16 @@ TestResult& checkStringEqual(TestResult& result,
  return result;
 }

+void checkStringEmpty(TestResult& result,


Instead of void, return a reference to result, to match checkStringEqual and allow for operator<< chaining.

BillyDonahue · 2018-06-05T14:35:12Z

src/test_lib_json/jsontest.h

@@ -239,6 +245,10 @@ TestResult& checkStringEqual(TestResult& result,
                             JsonTest::ToJsonString(actual), __FILE__,         \
                             __LINE__, #expected " == " #actual)

+/// \brief Asserts that string is empty.
+#define JSONTEST_ASSERT_STRING_EMPTY(actual)                                   \
+  JsonTest::checkStringEmpty(*result_, (actual), __FILE__, __LINE__, #actual);


No semicolon here.
These macros are supposed to fluently take trailing extra info added with operator<< on TestResult&.

BillyDonahue · 2018-06-05T14:57:41Z

src/jsontestrunner/main.cpp

  fseek(file, 0, SEEK_SET);
  JSONCPP_STRING text;
-  char* buffer = new char[size + 1];
+  auto* buffer = new char[size + 1];


The text string is sitting right here, willing to manage a memory block of char for us.
We can just use text's buffer space directly and skip buffer.

BillyDonahue · 2018-06-05T14:58:55Z

src/lib_json/json_reader.cpp

@@ -583,7 +583,7 @@ bool Reader::decodeNumber(Token& token, Value& decoded) {
    Char c = *current++;
    if (c < '0' || c > '9')
      return decodeDouble(token, decoded);
-    Value::UInt digit(static_cast<Value::UInt>(c - '0'));
+    auto digit(static_cast<Value::UInt>(c - '0'));


would be better for me with an = initializer.
auto digit = ...

BillyDonahue · 2018-06-05T15:02:17Z

src/lib_json/json_reader.cpp

@@ -1579,7 +1576,7 @@ bool OurReader::decodeNumber(Token& token, Value& decoded) {
    Char c = *current++;
    if (c < '0' || c > '9')
      return decodeDouble(token, decoded);
-    Value::UInt digit(static_cast<Value::UInt>(c - '0'));
+    auto digit(static_cast<Value::UInt>(c - '0'));


auto var = initializer;

BillyDonahue · 2018-06-05T15:12:59Z

src/lib_json/json_reader.cpp

-       itError != errors_.end(); ++itError) {
-    const ErrorInfo& error = *itError;
-    Reader::StructuredError structured;
+  allErrors.reserve(errors_.size());


std::vector<Reader::StructuredError> allErrors; allErrors.reserve(errors_.size()); std::transform(errors_.begin(), errors_.end(), std::back_inserter(allErrors), [](const ErrorInfo& e) -> Reader::StructuredError { return {e.token_.start_ - begin_, e.token_.end_ - begin_, e.message_}; }); return allErrors;

IMHO code without too much std boilerplate might be better
for (const ErrorInfo& e : errors_) {
allErrors.push_back(
{ e.token_.start_ - begin_, e.token_.end_ - begin_, e.message_ });
}

BillyDonahue · 2018-06-05T15:14:41Z

src/lib_json/json_reader.cpp

-       itError != errors_.end(); ++itError) {
-    const ErrorInfo& error = *itError;
-    OurReader::StructuredError structured;
+  allErrors.reserve(errors_.size());


same as Reader:: code above.

BillyDonahue · 2018-06-05T15:29:53Z

src/lib_json/json_value.cpp

@@ -107,7 +107,7 @@ static inline char* duplicateStringValue(const char* value, size_t length) {
  if (length >= static_cast<size_t>(Value::maxInt))
    length = Value::maxInt - 1;

-  char* newString = static_cast<char*>(malloc(length + 1));
+  auto* newString = static_cast<char*>(malloc(length + 1));


not much value here.

BillyDonahue · 2018-06-05T15:40:07Z

src/test_lib_json/jsontest.cpp

@@ -275,13 +273,12 @@ bool Runner::runAllTest(bool printSummary) const {
    }
    return true;
  } else {
-    for (unsigned int index = 0; index < failures.size(); ++index) {
-      TestResult& result = failures[index];
+    for (const TestResult& result : failures) {


You're adding constness here, and printFailure sounds like it might be or soon become non-const. let's leave off the const.

Rewrite readInputTestFile Small changes from pull request review

cdunn2001 · 2018-06-22T16:38:40Z

I'll let @BillyDonahue handle this.

And we probably cannot merge until TravisCI is fixed. (They seem to have dropped gcc-4.9. Not sure yet.)

cdunn2001 · 2018-06-24T23:50:22Z

TravisCI is fixed (with Meson for MSVC). This should be rebased to master. Then, if @BillyDonahue is fine with it, this can be merged.

hjmjohnson · 2019-01-14T22:42:16Z

@Nekto89 Can you please rebase this work on top of the current master branch and address the issues. I'm making a push to remove the backlog of PR issues.

If you can no longer work on this PR, just let me know and I'll close the PR.

hjmjohnson · 2019-01-14T23:33:03Z

@Nekto89 FYI: Superceeded by #865

Nekto89 added 2 commits June 4, 2018 23:51

Use empty() for emptiness checks.

bcd2680

Use for-range loops

eb09590

Use auto when declaring iterators Use auto when initializing with a cast to avoid duplicating the type name

Nekto89 changed the title ~~Use empty() for emptiness checks.~~ Code modernization (empty, for-range, auto) Jun 4, 2018

Add new test define JSONTEST_ASSERT_STRING_EMPTY

8080f7c

BillyDonahue requested changes Jun 5, 2018

View reviewed changes

Nekto89 added 2 commits June 5, 2018 23:17

Change declaration of checkStringEmpty

ac5494c

Rewrite readInputTestFile Small changes from pull request review

Add missing include for back_inserter

97c7001

cdunn2001 assigned BillyDonahue Jun 22, 2018

hjmjohnson closed this Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code modernization (empty, for-range, auto) #787

Code modernization (empty, for-range, auto) #787

Nekto89 commented Jun 4, 2018

BillyGoto commented Jun 4, 2018 via email

BillyDonahue Jun 5, 2018

BillyDonahue Jun 5, 2018

BillyDonahue Jun 5, 2018

BillyDonahue Jun 5, 2018

BillyDonahue Jun 5, 2018

BillyDonahue Jun 5, 2018

Nekto89 Jun 5, 2018

BillyDonahue Jun 5, 2018

BillyDonahue Jun 5, 2018

BillyDonahue Jun 5, 2018

cdunn2001 commented Jun 22, 2018

cdunn2001 commented Jun 24, 2018

hjmjohnson commented Jan 14, 2019

hjmjohnson commented Jan 14, 2019

Code modernization (empty, for-range, auto) #787

Code modernization (empty, for-range, auto) #787

Conversation

Nekto89 commented Jun 4, 2018

BillyGoto commented Jun 4, 2018 via email

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdunn2001 commented Jun 22, 2018

cdunn2001 commented Jun 24, 2018

hjmjohnson commented Jan 14, 2019

hjmjohnson commented Jan 14, 2019