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.

[gzliudan]

Sorry, I just noticed that index_ and storage_ is in a same union, so the right version maybe:

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

[gzliudan]

Maybe I got the clue:

struct StringStorage {
        DuplicationPolicy policy_: 2;
        unsigned length_: 30; // 1GB max
};

in C++ Builder: sizeof(StringStorage ) = 8, not 4, since DuplicationPolicy‘'s value is from 0 to 2, can't be hold in 2 bits int(1 bit is singal).

if I change StringStorage to:

struct StringStorage {
        unsigned policy_: 2;
        unsigned length_: 30; // 1GB max
};

then the sizeof(StringStorage ) = 4, so I suggest you check the size of StringStorage on your complier, I think you wish the sizeof it is 4 bytes, and on your compiler the size maybe 4. And at this time, my bug disappears also without modify Value::CZString::CZString(const CZString& other). But I still suggest modify it to let the code more clear.

[gzliudan]

void Value::CZString::swap(CZString& other) {
  std::swap(cstr_, other.cstr_);
  std::swap(index_, other.index_);
}

When the sizeof StringStorage is 8 bytes, this function will cause problem.

[cdunn2001]

Try pull #266, a much simpler fix maybe.

I don't mind also adding something more explicit for c_str==0, but if possible I want to solve the root problem first.

[cdunn2001]

Why closed? Did my small change in #266 solve the problem?

[gzliudan]

change source code from:

DuplicationPolicy policy_: 2;

to:

unsigned policy_: 2;

can fix the bug.

[gzliudan]

Victor Chen is the first man find the way of how run jsoncpp under C++ Builder:
http://www.cppfans.com/sdk/json/jsoncpp.asp

I only found this bug.

[cdunn2001]

Good. That is exactly the contents of the commit I linked.

Thanks for the info, and the link!

[gzliudan]

My name is Dan Liu (http://blog.csdn.net/gzliudan/article/details/45264201). This bug is found by me. Victor Chen is the first person tell us how to use jsoncpp under C++ Builder platform (http://www.cppfans.com/sdk/json/jsoncpp.asp).

[cdunn2001]

Oh, I'll try to change the commit message. I already put your username as the commit author.

[cdunn2001]

If you tell me your github e-mail, I can make the commit link back to your github account.

[cdunn2001]

Never mind. I don't want to alter the commits now that they're pushed to master. I'll include your name and that link in the next release message. (You can leave this open until then if you want.)

Thanks for you help, Dan!