dropbox / json11

A tiny JSON library for C++11.
MIT License
2.55k stars 613 forks source link

Interchangable string class #80

Closed jaw closed 7 years ago

jaw commented 7 years ago

I'm using a different string class than std::string. It would be good to add this either as a #define or a template parameter which defaults to std::string.

If so, is this something which you would accept into the code base?

Then, which method would you prefer?

If it would be a define, it should be something like:

ifndef JSON11_STRING_TYPE

define JSON11_STRING_TYPE std::string

endif

I have a domain-specific vector class as well, which might be useful to tweak as well...

j4cbo commented 7 years ago

I think this would be better off as a fork than built-in - it's a substantial code-ugliness / maintainability hit for something that's not useful for most users. @artwyman, thoughts?

artwyman commented 7 years ago

I don't have a strong opinion (given I introduced a similar hack in the unit tests) but I lean the same direction as @j4cbo. A macro for the class name will reduce readability, and also work only for people with a custom string type which has the same interface as std::string. On the flip side, a private fork for this would be pretty easy to maintain and merge, or alternatively a wrapper type around this one which did conversions. I think if making the string type configurable in this library, it might make sense to go farther with a policy template approach and a type alias, which would also be an approach which would let the people who want 64-bit ints parameterize that too (e.g. using Json=JsonGeneric<std::string, int64_t, double>;) but that approach also reduces readability in things like compiler errors, as has been proven with std::basic_string<> in the past, and is really a different goal than the current goal of json11, which includes being small and simple.

jaw commented 7 years ago

Alright, I will continue with my own fork then :) @artwyman the template idea sounds solid. My string class is designed to be mostly compatible with std::string, so it should probably work without modifications.

artwyman commented 7 years ago

Not directly related to this library, but I'm curious to hear what the key features are which lead to maintaining your own string class. We made one at my prior company in 2003 primarily for built-in Unicode support, but the amount of maintenance effort and risk of bugs has caused me to always lean away from the idea since, particularly now that the STL is more fleshed out and well supported than it was in 2003.

jaw commented 7 years ago

Yeah, this actually stems from back in 2004-2005 or so but has been maintained over the years. I still see a few advantages actually...

The initial motivation however is not relevant today - to generate a smaller binary for demoscene productions (linking with stl adds a few 100kb of bloat).

But unicode support definitely - the character type is a template param so char or wchar_t can be supplied for instance.

My string class internally uses a generic container (which I also wrote as a replacement for std::vector) with just the string-related methods on top, (https://github.com/vovoid/vsxu/blob/0.6.0/lib/common/include/string/vsx_string.h#L38) which means I can feed it a pointer inside a mmaped file. This in turn depends on the underlying container class having support for marking its data as volatile via a method call (which then does not attempt to free its data in its destructor) and a set_data() method. Maybe one can do this with some stl allocator tricks, but this still seems more straight forward to me.

Very useful for reading files from a DVD for instance.

By having full control over the storage container as well, one can pick an optimal re-allocation factor when it grows, I read some paper that it was best (on average) to grow the data allocated by 33%. https://github.com/vovoid/vsxu/blob/0.6.0/lib/common/include/container/vsx_nw_vector.h#L250

Some of this is trickery for sure, but this is geared towards performance for games and real time graphics.