Changeset View
Standalone View
source/blender/blenlib/BLI_serialize.hh
- This file was added.
| /* | |||||
| * This program is free software; you can redistribute it and/or | |||||
| * modify it under the terms of the GNU General Public License | |||||
| * as published by the Free Software Foundation; either version 2 | |||||
| * of the License, or (at your option) any later version. | |||||
| * | |||||
| * This program is distributed in the hope that it will be useful, | |||||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | |||||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | |||||
| * GNU General Public License for more details. | |||||
| * | |||||
| * You should have received a copy of the GNU General Public License | |||||
| * along with this program; if not, write to the Free Software Foundation, | |||||
| * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | |||||
| */ | |||||
| #pragma once | |||||
| /** \file | |||||
| * \ingroup bli | |||||
| * | |||||
| * An abstraction layer between serialization formats. Currently only | |||||
| * support JSON. | |||||
| */ | |||||
sybren: Linguistically this doesn't make much sense. This layer doesn't sit between serialization… | |||||
Done Inline Actionssupports sybren: supports | |||||
Done Inline ActionsI would add a bit more explanation on the very basics of the design. Mention how this can be used by other formats (by implementing a custom formatter using Formatter as base class), how data is modeled (what kind of primitives are supported and how ObjectValue can be used), is the order of items preserved when writing back to disk, how is versioning handled, etc. If it becomes more than a few lines then it can just become a link to the Wiki. Severin: I would add a bit more explanation on the very basics of the design. Mention how this can be… | |||||
| #include "BLI_map.hh" | |||||
| #include "BLI_string_ref.hh" | |||||
| #include "BLI_vector.hh" | |||||
| #include <ostream> | |||||
| #pragma once | |||||
Done Inline Actionsoeps sybren: oeps | |||||
| namespace blender::io::serialize { | |||||
| enum class eValueType { | |||||
| String, | |||||
| Int, | |||||
| Array, | |||||
Done Inline ActionsShouldn't this be \see? sybren: Shouldn't this be `\see`? | |||||
| Null, | |||||
| Boolean, | |||||
| Float, | |||||
| Object, | |||||
| }; | |||||
| class Value; | |||||
| class StringValue; | |||||
| class ObjectValue; | |||||
| template<typename T, eValueType V> class PrimitiveValue; | |||||
| using IntValue = PrimitiveValue<uint64_t, eValueType::Int>; | |||||
| using FloatValue = PrimitiveValue<double, eValueType::Float>; | |||||
Done Inline ActionsUsing uint64_t is error-prone for a type named IntValue with eValueType::Int. https://stackoverflow.com/a/13502497 suggests that JavaScript numbers roughly have a range of ±2**53, so int64_t will be sufficient. sybren: Using `uint64_t` is error-prone for a type named `IntValue` with `eValueType::Int`.
https… | |||||
| using BooleanValue = PrimitiveValue<bool, eValueType::Boolean>; | |||||
Done Inline ActionsAny reason to use float instead of double for JSON? This should be documented. sybren: Any reason to use `float` instead of `double` for JSON? This should be documented. | |||||
| template<typename Container, typename ContainerItem, eValueType V> class ContainerValue; | |||||
| using ArrayValue = | |||||
| ContainerValue<Vector<std::shared_ptr<Value>>, std::shared_ptr<Value>, eValueType::Array>; | |||||
Done Inline ActionsWhy are std::shared_ptrs used here? sybren: Why are `std::shared_ptr`s used here? | |||||
Done Inline ActionsDocument here, as people start reading from the top. sybren: Document here, as people start reading from the top. | |||||
| class Value { | |||||
| private: | |||||
Done Inline ActionsSomething as abstractly named as "Value" really needs documentation as to what it is and what it's for. sybren: Something as abstractly named as "Value" really needs documentation as to what it is and what… | |||||
| eValueType _type; | |||||
Done Inline ActionsCode-style says to use a trailing, not a leading _ for private/protected members: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Class_data_member_names Severin: Code-style says to use a trailing, not a leading `_` for private/protected members: https… | |||||
| protected: | |||||
| Value() = delete; | |||||
| Value(eValueType type) : _type(type) | |||||
| { | |||||
Done Inline ActionsEither add explicit or document why this constructor should not be explicit. sybren: Either add `explicit` or document why this constructor should not be `explicit`. | |||||
| } | |||||
| public: | |||||
| const eValueType type() const | |||||
| { | |||||
| return _type; | |||||
| } | |||||
| const StringValue *as_string_value() const; | |||||
| const IntValue *as_int_value() const; | |||||
| const FloatValue *as_float_value() const; | |||||
| const BooleanValue *as_boolean_value() const; | |||||
| const ArrayValue *as_array_value() const; | |||||
| const ObjectValue *as_object_value() const; | |||||
| }; | |||||
Done Inline ActionsI feel that these should get some documentation as to what they return, and in which scope they're valid. Is the code expected to check the type, before calling any of these? Who owns the returned pointer? sybren: I feel that these should get some documentation as to what they return, and in which scope… | |||||
Done Inline ActionsThese need documentation. What they do, but also what happens when the Value is not of the required type. Does that raise an exception? Return nulltpr? Produce invalid results? sybren: These need documentation. What they do, but also what happens when the Value is not of the… | |||||
| template<typename T, eValueType V> class PrimitiveValue : public Value { | |||||
| private: | |||||
| T _inner_value; | |||||
Done Inline ActionsDocumentation, certainly about the template variables. It also needs to explain what "primitive" means; after all, eValueType is not named ePrimitiveValueType, so there must be some value types that you cannot use here. sybren: Documentation, certainly about the template variables. It also needs to explain what… | |||||
| public: | |||||
Done Inline ActionsSame code style fix needed for these other private variables 😛 Severin: Same code style fix needed for these other private variables 😛 | |||||
| PrimitiveValue(const T value) : Value(V), _inner_value(value) | |||||
| { | |||||
| } | |||||
| const T value() const | |||||
| { | |||||
| return _inner_value; | |||||
| } | |||||
| }; | |||||
| class NullValue : public Value { | |||||
| public: | |||||
Done Inline ActionsRemove "Helper function to" sybren: Remove "Helper function to" | |||||
| NullValue() : Value(eValueType::Null) | |||||
| { | |||||
| } | |||||
| }; | |||||
| class StringValue : public Value { | |||||
| private: | |||||
| std::string _string; | |||||
| public: | |||||
| StringValue(const StringRef string) : Value(eValueType::String), _string(string) | |||||
| { | |||||
| } | |||||
| const std::string &string_value() const | |||||
| { | |||||
| return _string; | |||||
| } | |||||
| }; | |||||
| template<typename Container, typename ContainerItem, eValueType V> | |||||
| class ContainerValue : public Value { | |||||
| public: | |||||
| using Items = Container; | |||||
| using Item = ContainerItem; | |||||
| private: | |||||
| Container _inner_value; | |||||
| public: | |||||
| ContainerValue() : Value(V) | |||||
| { | |||||
| } | |||||
| const Container &elements() const | |||||
| { | |||||
| return _inner_value; | |||||
Done Inline ActionsRemove "Helper class for" sybren: Remove "Helper class for" | |||||
| } | |||||
| Container &elements() | |||||
| { | |||||
| return _inner_value; | |||||
Done Inline ActionsInstead of "internal" I'd call it "wrapped"; "internal" to me sounds too much like it's an implementation detail, but it's also part of the public interface of the type. sybren: Instead of "internal" I'd call it "wrapped"; "internal" to me sounds too much like it's an… | |||||
| } | |||||
| }; | |||||
| /** | |||||
| * Object is a key value container where the key must by a std::string. | |||||
| * Internally it is stored in a blender::Vector to ensure the order of keys. | |||||
| */ | |||||
| class ObjectValue : public ContainerValue<Vector<std::pair<std::string, std::shared_ptr<Value>>>, | |||||
| std::pair<std::string, std::shared_ptr<Value>>, | |||||
| eValueType::Object> { | |||||
Done Inline Actionsbe sybren: be | |||||
Done Inline Actionskey-value sybren: key-value | |||||
Done Inline ActionsBetter default-initialize primitive values: T inner_value_{};Otherwise on a default construction for example, this would be uninitialized. Severin: Better default-initialize primitive values:
```
T inner_value_{};
```
Otherwise on a default… | |||||
| public: | |||||
| /** Return a lookup map to quickly lookup by key. */ | |||||
Done Inline ActionsThis template is quite confusing, could we at least have an alias for std::pair<std::string, std::shared_ptr<Value>? Severin: This template is quite confusing, could we at least have an alias for `std::pair<std::string… | |||||
| const Map<std::string, std::shared_ptr<Value>> create_lookup() const | |||||
Done Inline ActionsThink this should be explicit. Severin: Think this should be `explicit`. | |||||
| { | |||||
| Map<std::string, std::shared_ptr<Value>> result; | |||||
| for (const Item &item : elements()) { | |||||
Done Inline ActionsWhy is this a std::shared_ptr? Who owns the values? Who is expected to be sharing the values? sybren: Why is this a `std::shared_ptr`? Who owns the values? Who is expected to be sharing the values? | |||||
| result.add_as(item.first, item.second); | |||||
| } | |||||
| return result; | |||||
| } | |||||
| }; | |||||
| class Formatter { | |||||
| public: | |||||
| virtual void serialize(std::ostream &os, Value &value) = 0; | |||||
| virtual Value *deserialize(std::istream &is) = 0; | |||||
| }; | |||||
| class JsonFormatter : public Formatter { | |||||
Done Inline ActionsThere must be a virtual destructor here, even if it's an abstract class. Severin: There must be a virtual destructor here, even if it's an abstract class. | |||||
| public: | |||||
Done Inline ActionsDocument sybren: Document | |||||
| uint8_t indentation_len = 0; | |||||
| public: | |||||
Done Inline ActionsWhy does the value need to be mutable in order to serialize it? What modifications will this function do? Either make it const, or use a pointer so that at the call itself it's clear that the function can modify the value. sybren: Why does the value need to be mutable in order to serialize it? What modifications will this… | |||||
| void serialize(std::ostream &os, Value &value) override; | |||||
| Value *deserialize(std::istream &is) override; | |||||
| }; | |||||
Done Inline ActionsDocument sybren: Document | |||||
| } // namespace blender::io::serialize | |||||
Done Inline ActionsUnless the full 0-255 range is needed, use int8_t instead. It'll give much more sensible erroneous results when there is some off-by-one error somewhere; invalid negative values can be tested for, but invalid wrapped-around-so-still-valid values cannot. sybren: Unless the full 0-255 range is needed, use `int8_t` instead. It'll give much more sensible… | |||||
Done Inline ActionsThis function should be named value() (so it's consistent with IntValue::value()) and not string_value(). Otherwise it's unclear when you see an invocation like this: j = value.as_string_value()->string_value(); as_string_value() already returns a "string value", so it's not immediately clear what different kind of "string value" would be returned by string_value(). sybren: This function should be named `value()` (so it's consistent with `IntValue::value()`) and not… | |||||
Not Done Inline ActionsThis seems a bit odd to me. I would expect the object to store items in a map itself, and a simple lookup function on the object directly. What's the reason for not doing this, to preserve the order of items? Is it really that important to preserve the order? If so, we could also store an index somehow and sort before writing. E.g. the object's map could be stored as Map<std::string, std::pair<std::unique_ptr<Value>, int>> -- maybe that makes internals more complex than they need to be, but at least the public API makes more sense to me. Not against the current solution, just bringing up an alternative for consideration. Severin: This seems a bit odd to me. I would expect the object to store items in a map itself, and a… | |||||
Done Inline ActionsYes I considered it. Goal was a minialistic abstration layer. Ordering of data is a more complex topic. There is no default ordering. JSON for example orders the keys in alphabetic order. Other formatters can decide a different order. For our situation alphabetic would increase the index filesizes to create a better human readable index. I have chosen to don't sort at all and let the order of the keys be based by the caller. jbakker: Yes I considered it. Goal was a minialistic abstration layer.
I agree that the public API is… | |||||
Done Inline ActionsJSON specs are also not 100% clear about duplicated keys inside a JSON file. jbakker: JSON specs are also not 100% clear about duplicated keys inside a JSON file.
It should be, but… | |||||
Done Inline ActionsVery minor suggestion: I prefer using the single line syntax for such short doxygen comments: /** Short description, blah blah. */ Severin: Very minor suggestion: I prefer using the single line syntax for such short doxygen comments… | |||||
Linguistically this doesn't make much sense. This layer doesn't sit between serialization formats. I'd say "abstraction layer for serialization formats".