Bug: can't create array more than 5 items

[gzliudan]

test code is:

    int count = 5;
    Json::Value root;
    Json::Value item;
    root["array"] = Json::Value::nullRef;
    for (int i = 0; i < count; i++)
    {
        item["a"] = i;
        item["b"] = i;
        root["array"][i] = item;
    }

the above code's result is wrong,

{
   "array" : [
      {
         "a" : 0,
         "b" : 0
      }
   ]
}

but if when the count = 4, then the result is right:

{
   "array" : [
      {
         "a" : 0,
         "b" : 0
      },
      {
         "a" : 1,
         "b" : 1
      },
      {
         "a" : 2,
         "b" : 2
      },
      {
         "a" : 3,
         "b" : 3
      }
   ]
}

[cdunn2001]

WFM:

{
        "array" :
        [
                {
                        "a" : 0,
                        "b" : 0
                },
                {
                        "a" : 1,
                        "b" : 1
                },
                {
                        "a" : 2,
                        "b" : 2
                },
                {
                        "a" : 3,
                        "b" : 3
                },
                {
                        "a" : 4,
                        "b" : 4
                }
        ]
}

I used your code exactly. I tried both master and 0.y.z on Ubuntu 14.04 (Trusty) with g++-4.8.2.

Which commit? What O/S? What compiler? Have you run valgrind, as this could be your own bug? Did you definitely do a clean build? What did you use to display the contents of root?

[gzliudan]

OS: Windows 7 64 bit
Compiler: embarcadero RAD Studio C++ Builder XE7 Update 1
Target platforms: windows 32 bit, debug or release
jsoncpp branch: 0.y.z(0.10.2)

below is my complete code for test:
void __fastcall TFormTestJson::ButtonJsonEncodeClick(TObject *Sender)
{
int count = 10;
Json::Value root;
Json::Value item;
for (int i = 0; i < count; i++)
{
item["a"] = i;
item["b"] = i;
root[i] = item;
}

UTF8String Utf8Text = Json::FastWriter().write(root).c_str();
EditJsonStr->Text = Utf8Text;

Utf8Text = root.toStyledString().c_str();
Memo1->Text = Utf8Text;

}

when the count is 10， the result is:
[
{
"a" : 4,
"b" : 4
},
{
"a" : 1,
"b" : 1
}
]

[cdunn2001]

Well, I believe you, but it's hard to debug when I can't repeat it. Can you trap it in your own debugger?

[gzliudan]

en, I will try to debug this problem in my IDE in this weekend.

[gzliudan]

I add some debug lines to ArrayIndex Value::size() const function as below:

ArrayIndex Value::size() const {
  switch (type_) {
  case nullValue:
  case intValue:
  case uintValue:
  case realValue:
  case booleanValue:
  case stringValue:
    return 0;
  case arrayValue: // size of the array is highest index + 1
    if (!value_.map_->empty()) {
        ObjectValues::const_iterator itLast = value_.map_->end();
        --itLast;

        ObjectValues::const_iterator it =  value_.map_->begin();
        int temp_for_debug =  0;
        for (; it != value_.map_->end(); it++)
        {
            temp_for_debug = it->first.index();
        }

        return (*itLast).first.index() + 1;
    }
    return 0;
  case objectValue:
    return ArrayIndex(value_.map_->size());
  }
  JSON_ASSERT_UNREACHABLE;
  return 0; // unreachable;
}

when value_.map_ has 10 items, temp_for_debug's value changes as below:
0 1 2 3 4 0 1 2 3 4
why can't return value_.map_->size() here?

[cdunn2001]

The logic is correct. The map item keys should be 0..9. Something has corrupted your map. I suspect our use of erase() elsewhere in that file. Could you step through your code?

[gzliudan]

The reason is found! I resolved the problem by add below line to function Value::CZString::CZString(const CZString& other)

if (!cstr_)
{
    this->index_ = other.index_;
}

then the whole function code is below:

Value::CZString::CZString(const CZString& other)
    : cstr_(other.storage_.policy_ != noDuplication && other.cstr_ != 0
                ? duplicateStringValue(other.cstr_, other.storage_.length_)
                : other.cstr_)
{
    if (!cstr_)
    {
        this->index_ = other.index_;
    }

  storage_.policy_ = (other.cstr_
                 ? (other.storage_.policy_ == noDuplication
                     ? noDuplication : duplicate)
                 : other.storage_.policy_);
  storage_.length_ = other.storage_.length_;
}

[gzliudan]

I think the right code should be：

Value::CZString::CZString(const CZString& other)
    : cstr_(other.storage_.policy_ != noDuplication && other.cstr_ != 0
       ? duplicateStringValue(other.cstr_, other.storage_.length_)
        : other.cstr_)
{
     this->index_ = other.index_;

   storage_.policy_ = (other.cstr_  ? (other.storage_.policy_ == noDuplication
       ? noDuplication : duplicate)  : other.storage_.policy_);
   storage_.length_ = other.storage_.length_;
}

[cdunn2001]

You're right, but I want to understand why it has not affected others. Maybe you have an optimized build? Maybe the debug build initializes cstr_ to 0 consistently. Something like that. Any other information you learn will be welcome. I'll try to build a test-case later. (I don't have time to look at it closely today.)

[gzliudan]

I‘m using c++ Builder XE7UP1 32bit which does not support C++11, and jsoncpp 0.10.2(0.y.z branch). The problem appears both in debug and release version. Even there's only one item in json array, if the item is an big object, it's value maybe null sometimes. And the problem will appears certainly if I insert more than 5 items into json array. I didn't optimized your source code, except to modify source code to let it compatible with C++ Builder. C++ Builder integrates dinkumware's STL include map. Maybe C++ Builder'users is small. I will show you how to modify the source code to support C++ Builder in next comments.

[gzliudan]

first, modify value.h:

move class ValueIteratorBase's constructor function to the end of class, otherwise dinkumware's map header will complain error when compile. And add public keyword before ValueIteratorBase's constructor after move. The whole class should as below:

class JSON_API ValueIteratorBase {
public:
  typedef std::bidirectional_iterator_tag iterator_category;
  typedef unsigned int size_t;
  typedef int difference_type;
  typedef ValueIteratorBase SelfType;

    bool operator==(const SelfType& other) const { return isEqual(other); }

    bool operator!=(const SelfType& other) const { return !isEqual(other); }

    difference_type operator-(const SelfType& other) const {
        return other.computeDistance(*this);
    }

    /// Return either the index or the member name of the referenced value as a
    /// Value.
    Value key() const;

    /// Return the index of the referenced Value, or -1 if it is not an arrayValue.
    UInt index() const;

    /// Return the member name of the referenced Value, or "" if it is not an
    /// objectValue.
    /// \note Avoid `c_str()` on result, as embedded zeroes are possible.
    std::string name() const;

    /// Return the member name of the referenced Value. "" if it is not an
    /// objectValue.
    /// \deprecated This cannot be used for UTF-8 strings, since there can be embedded nulls.
    JSONCPP_DEPRECATED("Use `key = name();` instead.")
    char const* memberName() const;
    /// Return the member name of the referenced Value, or NULL if it is not an
    /// objectValue.
    /// \note Better version than memberName(). Allows embedded nulls.
    char const* memberName(char const** end) const;

protected:
    Value& deref() const;

    void increment();

    void decrement();

    difference_type computeDistance(const SelfType& other) const;

    bool isEqual(const SelfType& other) const;

    void copy(const SelfType& other);

private:
    Value::ObjectValues::iterator current_;
    // Indicates that iterator is for a null value.
    bool isNull_;

public:
    ValueIteratorBase();
    explicit ValueIteratorBase(const Value::ObjectValues::iterator& current);
};

[gzliudan]

second, modify json_writer.cpp, add below lines in the front of file:

#if defined(__BORLANDC__)
#include <float.h>
#define isfinite _finite
#define snprintf _snprintf
#endif

such as:

#if !defined(JSON_IS_AMALGAMATION)
    #include <jsoncpp/json_writer.h>
    #include <jsoncpp/json_tool.h>
#endif // if !defined(JSON_IS_AMALGAMATION)
#include <iomanip>
#include <memory>
#include <sstream>
#include <utility>
#include <set>
#include <cassert>
#include <cstring>
#include <cstdio>

#if defined(_MSC_VER) && _MSC_VER >= 1200 && _MSC_VER < 1800 // Between VC++ 6.0 and VC++ 11.0
    #include <float.h>
    #define isfinite _finite
#elif defined(__sun) && defined(__SVR4) //Solaris
    #include <ieeefp.h>
    #define isfinite finite
#elif defined(__BORLANDC__)
    #include <float.h>
    #define isfinite _finite
    #define snprintf _snprintf
#else
    #include <cmath>
    #define isfinite std::isfinite
#endif

[gzliudan]

The above tow steps are all job to run under CB. In addition, I don't need ’\n‘ character on the end of line according to my application's protocol, so I modify FastWriter::write function also as below:

std::string FastWriter::write(const Value& root) {
  document_ = "";
  writeValue(root);
  // document_ += "\n"; // comment by liudan on 2015-04-12
  return document_;
}

[cdunn2001]

For backward-compatibility, we cannot modify FastWriter. But that's deprecated anyway. You should be using StreamWriterBuilder, where the trailing newline is configurable.

[cdunn2001]

I'll try to apply those other changes when I get a chance. They're to eliminate warnings, right?

The proposed fix makes sense too, but like I said, I really want to understand what is going on. I at least want to understand why I cannot reproduce your failure, and why valgrind sees no problem.