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 for serialization formats. | |||||
| * | |||||
| * Allowing to read/write data to a serialization format like 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 <ostream> | |||||
| #include "BLI_map.hh" | |||||
| #include "BLI_string_ref.hh" | |||||
| #include "BLI_vector.hh" | |||||
| namespace blender::io::serialize { | |||||
Done Inline Actionsoeps sybren: oeps | |||||
| /** | |||||
| * Enumeration containing all sub-classes of Value. It is used as for type checking. | |||||
| * | |||||
| * /see #Value::type() | |||||
| */ | |||||
Done Inline ActionsShouldn't this be \see? sybren: Shouldn't this be `\see`? | |||||
| enum class eValueType { | |||||
| String, | |||||
| Int, | |||||
| Array, | |||||
| Null, | |||||
| Boolean, | |||||
| Double, | |||||
| Object, | |||||
| }; | |||||
| class Value; | |||||
| class StringValue; | |||||
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… | |||||
| class ObjectValue; | |||||
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 T, eValueType V> class PrimitiveValue; | |||||
| using IntValue = PrimitiveValue<int64_t, eValueType::Int>; | |||||
| using DoubleValue = PrimitiveValue<double, eValueType::Double>; | |||||
| using BooleanValue = PrimitiveValue<bool, eValueType::Boolean>; | |||||
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. | |||||
| template<typename Container, typename ContainerItem, eValueType V> class ContainerValue; | |||||
| using ArrayValue = | |||||
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… | |||||
| ContainerValue<Vector<std::shared_ptr<Value>>, std::shared_ptr<Value>, eValueType::Array>; | |||||
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… | |||||
| /** | |||||
| * Class containing a (de)serializable value. | |||||
| * | |||||
| * To serialize from or to a specific format the Value will be used as an intermediate container | |||||
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`. | |||||
| * holding the values. Value class is abstract. There are concreate classes to for different data | |||||
| * types. | |||||
| * | |||||
| * - `StringValue`: contains a string. | |||||
| * - `IntValue`: contains an integer. | |||||
| * - `ArrayValue`: contains an array of elements. Elements don't need to be the same type. | |||||
| * - `NullValue`: represents nothing (null pointer or optional). | |||||
| * - `BooleanValue`: contains a boolean (true/false). | |||||
| * - `DoubleValue`: contains a double precision floating point number. | |||||
| * - `ObjectValue`: represents an object (key value pairs where keys are strings and values can be | |||||
| * of different types. | |||||
| * | |||||
| */ | |||||
| class Value { | |||||
| private: | |||||
| eValueType type_; | |||||
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… | |||||
| protected: | |||||
| Value() = delete; | |||||
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… | |||||
| explicit Value(eValueType type) : type_(type) | |||||
| { | |||||
Done Inline ActionsSame code style fix needed for these other private variables 😛 Severin: Same code style fix needed for these other private variables 😛 | |||||
| } | |||||
| public: | |||||
| virtual ~Value() = default; | |||||
| const eValueType type() const | |||||
| { | |||||
| return type_; | |||||
| } | |||||
| /** | |||||
| * Helper function to cast to a StringValue. | |||||
| * Will return nullptr when it is a different type. | |||||
Done Inline ActionsRemove "Helper function to" sybren: Remove "Helper function to" | |||||
| */ | |||||
| const StringValue *as_string_value() const; | |||||
| /** | |||||
| * Helper function to cast to an IntValue. | |||||
| * Will return nullptr when it is a different type. | |||||
| */ | |||||
| const IntValue *as_int_value() const; | |||||
| /** | |||||
| * Helper function to cast to a DoubleValue. | |||||
| * Will return nullptr when it is a different type. | |||||
| */ | |||||
| const DoubleValue *as_double_value() const; | |||||
| /** | |||||
| * Helper function to cast to a BooleanValue. | |||||
| * Will return nullptr when it is a different type. | |||||
| */ | |||||
| const BooleanValue *as_boolean_value() const; | |||||
| /** | |||||
| * Helper function to cast to an ArrayValue. | |||||
| * Will return nullptr when it is a different type. | |||||
| */ | |||||
| const ArrayValue *as_array_value() const; | |||||
| /** | |||||
| * Helper function to cast to an ObjectValue. | |||||
| * Will return nullptr when it is a different type. | |||||
| */ | |||||
| const ObjectValue *as_object_value() const; | |||||
| }; | |||||
| /** | |||||
| * Helper class for generating value types that represent types that are typically known processor | |||||
| * data types. | |||||
Done Inline ActionsRemove "Helper class for" sybren: Remove "Helper class for" | |||||
| */ | |||||
| template< | |||||
| /** | |||||
| * Internal c/cpp data type that is used to store the 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… | |||||
| typename T, | |||||
| /** | |||||
| * Value type of the class. | |||||
| */ | |||||
| eValueType V> | |||||
| class PrimitiveValue : public Value { | |||||
| private: | |||||
| T inner_value_; | |||||
| public: | |||||
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… | |||||
| PrimitiveValue(const T value) : Value(V), inner_value_(value) | |||||
| { | |||||
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… | |||||
| } | |||||
Done Inline ActionsThink this should be explicit. Severin: Think this should be `explicit`. | |||||
| const T value() const | |||||
| { | |||||
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? | |||||
| return inner_value_; | |||||
| } | |||||
| }; | |||||
| class NullValue : public Value { | |||||
| public: | |||||
| NullValue() : Value(eValueType::Null) | |||||
| { | |||||
| } | |||||
| }; | |||||
| class StringValue : public Value { | |||||
| private: | |||||
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. | |||||
| std::string string_; | |||||
Done Inline ActionsDocument sybren: Document | |||||
| public: | |||||
| StringValue(const StringRef string) : Value(eValueType::String), string_(string) | |||||
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… | |||||
| { | |||||
| } | |||||
| const std::string &string_value() const | |||||
Done Inline ActionsDocument sybren: Document | |||||
| { | |||||
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… | |||||
| return string_; | |||||
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… | |||||
| } | |||||
| }; | |||||
| /** | |||||
| * Helper template for arrays and objects. | |||||
| * | |||||
| * Both ArrayValue and ObjectValue store their values in an array. | |||||
| */ | |||||
| template< | |||||
| /** | |||||
| * The container type where the elements are stored in. | |||||
| */ | |||||
| typename Container, | |||||
| /** | |||||
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… | |||||
| * Type of the data inside the container. | |||||
| */ | |||||
| typename ContainerItem, | |||||
| /** | |||||
| * ValueType representing the value (object/array). | |||||
| */ | |||||
| 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_; | |||||
| } | |||||
| Container &elements() | |||||
| { | |||||
| return inner_value_; | |||||
| } | |||||
| }; | |||||
| /** | |||||
| * Internal storage type for ObjectValue. | |||||
| * | |||||
| * The elements are stored as an key value pair. The value is a shared pointer so it can be shared | |||||
| * when using `ObjectValue::create_lookup`. | |||||
| */ | |||||
| using ObjectElementType = std::pair<std::string, std::shared_ptr<Value>>; | |||||
| /** | |||||
| * Object is a key-value container where the key must be a std::string. | |||||
| * Internally it is stored in a blender::Vector to ensure the order of keys. | |||||
| */ | |||||
| class ObjectValue | |||||
| : public ContainerValue<Vector<ObjectElementType>, ObjectElementType, eValueType::Object> { | |||||
| public: | |||||
| using LookupValue = std::shared_ptr<Value>; | |||||
| using Lookup = Map<std::string, LookupValue>; | |||||
| /** | |||||
| * Return a lookup map to quickly lookup by key. | |||||
| * | |||||
| * The lookup is owned by the caller. | |||||
| */ | |||||
| const Lookup create_lookup() const | |||||
| { | |||||
| Lookup result; | |||||
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… | |||||
| for (const Item &item : elements()) { | |||||
| result.add_as(item.first, item.second); | |||||
| } | |||||
| return result; | |||||
| } | |||||
| }; | |||||
| /** | |||||
| * Interface for any provided Formatter. | |||||
| */ | |||||
| class Formatter { | |||||
| public: | |||||
| virtual ~Formatter() = default; | |||||
| /** | |||||
| * Serialize the value to the given stream. | |||||
| */ | |||||
| virtual void serialize(std::ostream &os, const Value &value) = 0; | |||||
| /** | |||||
| * Deserialize the stream. | |||||
| */ | |||||
| virtual std::unique_ptr<Value> deserialize(std::istream &is) = 0; | |||||
| }; | |||||
| /** | |||||
| * Formatter to (de)serialize a json formatted stream. | |||||
| */ | |||||
| class JsonFormatter : public Formatter { | |||||
| public: | |||||
| /** | |||||
| * The identation level to use. | |||||
| * Typically number of chars. Set to 0 to not use identation. | |||||
| */ | |||||
| int8_t indentation_len = 0; | |||||
| public: | |||||
| void serialize(std::ostream &os, const Value &value) override; | |||||
| std::unique_ptr<Value> deserialize(std::istream &is) override; | |||||
| }; | |||||
| } // namespace blender::io::serialize | |||||
Linguistically this doesn't make much sense. This layer doesn't sit between serialization formats. I'd say "abstraction layer for serialization formats".