open-source-parsers / jsoncpp

A C++ library for interacting with JSON.
Other
8.06k stars 2.63k forks source link

Why are the branches of the if statement not implemented consistently? #1520

Open pjljvandelaar opened 9 months ago

pjljvandelaar commented 9 months ago

Dear JSONCPP developers,

I was looking at the following if statement starting on https://github.com/open-source-parsers/jsoncpp/blob/69098a18b9af0c47549d9a271c054d13ca92b006/src/lib_json/json_writer.cpp#L734

It took me some time to realize that the rather complex code in the then branch: https://github.com/open-source-parsers/jsoncpp/blob/69098a18b9af0c47549d9a271c054d13ca92b006/src/lib_json/json_writer.cpp#L738-L757 is equal to joining the elements in the range [0,size) where size != 0 with a separator, and a functional equivalent, yet simpler, implementation is

      for (unsigned index = 0; index < size ; ++index) {
        const Value& childValue = value[index];
        writeCommentBeforeValue(childValue);
        if (hasChildValue)
          writeWithIndent(childValues_[index]);
        else {
          if (!indented_)
            writeIndent();
          indented_ = true;
          writeValue(childValue);
          indented_ = false;
        }
        if (index < size -1) {
           *document_ << ",";
        }
        writeCommentAfterValueOnSameLine(childValue);
      }

I assume performance (saving a comparison) is the reason for the extra complexity.

Yet, when looking at the else branch https://github.com/open-source-parsers/jsoncpp/blob/69098a18b9af0c47549d9a271c054d13ca92b006/src/lib_json/json_writer.cpp#L764-L768 that is not optimized, like the then branch, to

      unsigned index = 0;
      for (;;) {
        *document_ << childValues_[index];
        if (++index == size)
        break;
        *document_ << ", ";
      }

So I wonder

  1. what is more important for jsoncpp code: readability or performance?
  2. Shouldn't these two branches not be implemented consistently?

Thanks in advance for your answers (and possibly code improvements)!