search

nlohmann / std_json

A proposal for a JSON data type in the C++ standard library
54 stars 1 forks source link

first comments #10

Closed nlohmann closed 7 years ago

nlohmann commented 7 years ago

A lot of small comments and edits. I also added some TODOs to discuss some things.

I have some more remarks beginning in the section where push_back is defined. I shall add them tomorrow.

mariokonrad commented 7 years ago

First: thank you for the spelling corrections, at least some mistakes are consistently ;-)

Discussion points:

  1. Order of enumeration json_type: the old order was increasing complexity of the data. Similarity to python is also OK. Personally, I have no preference.

  2. Equality of floating point numbers: maybe the equality could be formulated as the difference must be smaller or equal to std::numeric_limits<T>::epsilon(). The construction should catch inf and NAN, to prevent the necessity of checks in comparisons. They are forbidden by JSON anyway. Any thoughts?

  3. front() and back(): indeed begin()/end() have constant complexity, so could front(). Depending on "special" implementations of containers, back() could be more complex than end(), e.g. trees. But since both, front() and back() rely on the underlying type, I think there cannot be other guarantees made by basic_json.

  4. clear() to default construct with the same type: semantics, points could be made for both positions... Since the proposal is based on nlomann/json, let's go with it's behavior.

nlohmann commented 7 years ago

Regarding https://github.com/nlohmann/std_json/pull/10#issuecomment-324148502:

  1. Ok. (Removed the TODO)
  2. I have a similar discussion (https://github.com/nlohmann/json/issues/703) right now. I'm not sure how much a wrapper around a floating-point number should mess with equality tests - especially, when it uses the operator== overload.
  3. Ok. (Removed the TODO)
  4. Ok. I shall make the changes.
mariokonrad commented 7 years ago

I suggest to merge and keep nlohmann/json#703 as an open issue.