This fixes issue 1302 (simdjson#1303)

lemire · web-flow · commit 1abb64f6f658 · 2020-11-27T14:19:36.000-05:00
* This verifies issue 1302

* I believe that this fixes the issue.

* Let us do it differently.

* Better comments.
diff --git a/include/simdjson/dom/document_stream-inl.h b/include/simdjson/dom/document_stream-inl.h
@@ -116,15 +116,39 @@ simdjson_really_inline document_stream::iterator::iterator(document_stream& _str
 }
 
 simdjson_really_inline simdjson_result<element> document_stream::iterator::operator*() noexcept {
-  // Once we have yielded any errors, we're finished.
-  if (stream.error) { finished = true; return stream.error; }
+  // Note that in case of error, we do not yet mark
+  // the iterator as "finished": this detection is done
+  // in the operator++ function since it is possible
+  // to call operator++ repeatedly while omitting
+  // calls to operator*.
+  if (stream.error) { return stream.error; }
   return stream.parser->doc.root();
 }
 
 simdjson_really_inline document_stream::iterator& document_stream::iterator::operator++() noexcept {
+  // If there is an error, then we want the iterator
+  // to be finished, no matter what. (E.g., we do not
+  // keep generating documents with errors, or go beyond
+  // a document with errors.)
+  //
+  // Users do not have to call "operator*()" when they use operator++,
+  // so we need to end the stream in the operator++ function.
+  //
+  // Note that setting finished = true is essential otherwise
+  // we would enter an infinite loop.
+  if (stream.error) { finished = true; }
+  // Note that stream.error() is guarded against error conditions
+  // (it will immediately return if stream.error casts to false).
+  // In effect, this next function does nothing when (stream.error)
+  // is true (hence the risk of an infinite loop).
   stream.next();
   // If that was the last document, we're finished.
+  // It is the only type of error we do not want to appear
+  // in operator*.
   if (stream.error == EMPTY) { finished = true; }
+  // If we had any other kind of error (not EMPTY) then we want
+  // to pass it along to the operator* and we cannot mark the result
+  // as "finished" just yet.
   return *this;
 }
 
@@ -168,6 +192,7 @@ simdjson_really_inline std::string_view document_stream::iterator::source() cons
 
 
 inline void document_stream::next() noexcept {
+  // We always enter at once once in an error condition.
   if (error) { return; }
 
   // Load the next document from the batch
diff --git a/tests/document_stream_tests.cpp b/tests/document_stream_tests.cpp
@@ -69,6 +69,22 @@ namespace document_stream_tests {
     }
     return true;
   }
+
+  bool test_naked_iterators() {
+    std::cout << "Running " << __func__ << std::endl;
+    auto json = R"([1,23] "lone string" {"key":"unfinished value}  )"_padded;
+    simdjson::dom::parser parser;
+    simdjson::dom::document_stream stream;
+    ASSERT_SUCCESS( parser.parse_many(json).get(stream) );
+    size_t count = 0;
+    // We do not touch the document, intentionally.
+    for(auto i = stream.begin(); i != stream.end(); ++i) {
+      if(count > 10) { break; }
+      count++;
+    }
+    return count == 1;
+  }
+
   bool single_document() {
     std::cout << "Running " << __func__ << std::endl;
     simdjson::dom::parser parser;
@@ -322,7 +338,8 @@ namespace document_stream_tests {
   }
 
   bool run() {
-    return test_current_index()  &&
+    return test_naked_iterators() &&
+           test_current_index()  &&
            single_document() &&
 #if SIMDJSON_EXCEPTIONS
            single_document_exceptions() &&
diff --git a/tests/parse_many_test.cpp b/tests/parse_many_test.cpp
@@ -73,7 +73,6 @@ bool validate(const char *dirname) {
             auto error = simdjson::padded_string::load(fullpath).get(json);
             if (!error) {
                 simdjson::dom::parser parser;
-
                 ++how_many;
                 simdjson::dom::document_stream docs;
                 error = parser.parse_many(json).get(docs);
