Don't include <iostream>, use std::make_shared

[olegendo]

I've been evaluating this library for usage in an embedded software that runs on an MCU with on-chip flash. Memory on these devices is a premium. Just including the <iostream> file, makes the ROM image grow by about 200 KByte, and I've got only about 768 KByte in total. This is a known problem of <iostream>. One workaround is not to include it in (library) header files, but only in source files where it is actually needed. Header files should include <iosfwd> instead.

While skimming over the code, I've also spotted some opportunities to use std::make_shared.

Tested with 'make' and 'make test' on GCC 5.4.0 x86_64-linux-gnu.
Also briefly checked that the library can be used without including <iostream> on GCC 6.4.1 rx-elf.

[mention-bot]

@olegendo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nlohmann and @vasild to be potential reviewers.

[coveralls]

Coverage remained the same at 100.0% when pulling e3bb156 on olegendo:develop into f1c543c on nlohmann:develop.

[nlohmann]

Looks good to me.

[nlohmann]

Thanks a lot!

Can you comment on the memory usage of the library? Is it usable in your usecase?

[olegendo]

Ugh, sorry, I didn't notice the examples.

I can't comment on the memory footprint of the library yet. The <iostream> was a no-go for my case. But now I can try using it for some data exchange between the embedded http server and the browser.

I'm wondering why you haven't used a base class and virtual functions for json_value instead of the open coded type-switch-cases? At the first glance, it looks like it could chop off quite a bit of code, but I might be missing something here. What's the reasoning behind the choice of data layout for class basic_json?

Notice that the statement in the "Memory efficiency" section of README.md is not complete. sizeof (basic_json) is greater than sizeof (void*) + sizeof (uint8_t), because of struct alignment rules.

[nlohmann]

Good question. I think such a base class would be handy when it comes to using different types, because instead of assuming string types to have the basic_string interface, we could make this explicit (this may be relevant to #474 (comment)). I'm not sure about the memory requirements though.

In any case, the library just "evolved" into this state. Using a union was perfectly reasonable when I started.

[jaredgrubb]

Great patch!

[olegendo]

What I meant is something like that:

struct value_base
{
  virtual ~value_base (void) { }
  virtual bool empty (void) const noexcept = 0;
  virtual size_type size (void) const noexcept  = 0;
  virtual void clear (void) noexcept = 0;
  ...
};

class string_value : public value_base
{
  public:
    virtual bool empty (void) const noexcept override { return m_value.empty (); }
    virtual size_type size (void) const noexcept  override { return 1; }
    virtual clear (void) noexcept override { m_value.clear (); }
    ...
  private:
    std::string m_value;
};

class boolean_value : public value_base
{
public:
    virtual bool empty (void) const noexcept override { return false; }
    virtual size_type size (void) const noexcept  override { return 1; }
    virtual clear (void) noexcept override { m_value = false; }
    ...
private:
   bool m_value;
};
...

But I just noticed that you store bool and number types by value in the union. This brings up the idea of storing all the various value types by-value in basic_json (via std::aligned_storage). On a 32 bit target and libstdc++, sizeof (json) = 12 bytes, sizeof (std:.vector) = 12 bytes, sizeof (std::string) 24 bytes, sizeof (std::map) = 24 bytes. So sizeof (json) would become 28 bytes, which means json numbers would take double the space than before. On the other hand, it would eliminate one indirection when accessing json objects, arrays and strings. Also, for each json object/array/string the heap management overhead per allocation should be taken into account (~8 bytes), which would be eliminated.

Is it worth going down that rabbit hole? Hmm ... If you have access to some benchmarks, I could give it a try, just out of curiosity.

[nlohmann]

This sounds interesting! Related: #456 #474

[nlohmann]

I thought about this idea. It would mean a complete refactoring of the library. For instance, we would need to move the serialization implementation to the individual classes, and also a lot of the internal conversions which currently depend on the mere types (e.g., any kind of string representation can be somehow converted to string_t - we would now require these constructors in the string_value class).

It is a very interesting approach, and I shall open a feature branch. But I am not sure how long it would take to be able to really use it.

(Or maybe I am just missing something and things are much easier ;-))

[nlohmann]

I opened a feature branch: c1f4b24. There are a lot of open questions (at least to me), but it's late and I'm not in a rush.

[pboettch]

@olegendo Completely off-topic to this thread, is your project, where you are trying to use this library, open-source?

[olegendo]

@pboettch Sorry, no. Unfortunately it's not open source.

[olegendo]

@nlohmann I have also created an experimental branch here https://github.com/olegendo/json/tree/virtual_funcs

Unfortunately, I can't get it to fully compile yet. Somehow the to_json machinery is broken ... but I hope that it shows my intention.
I haven't done a lot of replacements that could be done with the json_value_* classes in place and left the type-switches. Even if this might not make it into mainline as-is, I think it could be a good idea to wrap the direct member accesses (m_type, m_value.number_float and so on) in functions. It will make future refactoring work easier.