Differential D12544 Diff 42915 source/blender/blenlib/BLI_serialize.hh

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.
				sybrenUnsubmitted Done Inline Actions Linguistically this doesn't make much sense. This layer doesn't sit between serialization formats. I'd say "abstraction layer for serialization formats". sybren: Linguistically this doesn't make much sense. This layer doesn't sit between serialization…
				sybrenUnsubmitted Done Inline Actions supports sybren: supports
				*/

				SeverinUnsubmitted Done Inline Actions I 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

				sybrenUnsubmitted Done Inline Actions oeps sybren: oeps
				namespace blender::io::serialize {

				enum class eValueType {
				String,
				Int,
				Array,
				sybrenUnsubmitted Done Inline Actions Shouldn'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>;
				sybrenUnsubmitted Done Inline Actions Using `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 ±253, so `int64_t` will be sufficient. sybren:** Using `uint64_t` is error-prone for a type named `IntValue` with `eValueType::Int`. https…
				using FloatValue = PrimitiveValue<double, eValueType::Float>;
				sybrenUnsubmitted Done Inline Actions Any 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.
				using BooleanValue = PrimitiveValue<bool, eValueType::Boolean>;

				template<typename Container, typename ContainerItem, eValueType V> class ContainerValue;
				using ArrayValue =
				ContainerValue<Vector<std::shared_ptr<Value>>, std::shared_ptr<Value>, eValueType::Array>;

				sybrenUnsubmitted Done Inline Actions Why are `std::shared_ptr`s used here? sybren: Why are `std::shared_ptr`s used here?
				sybrenUnsubmitted Done Inline Actions Document here, as people start reading from the top. sybren: Document here, as people start reading from the top.
				class Value {
				sybrenUnsubmitted Done Inline Actions Something 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…
				private:
				eValueType type_;

				SeverinUnsubmitted Done Inline Actions Code-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)
				sybrenUnsubmitted Done Inline Actions Either 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:
				virtual ~Value() = default;
				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;
				sybrenUnsubmitted Done Inline Actions These 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…
				};
				sybrenUnsubmitted Done Inline Actions I 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…

				template<typename T, eValueType V> class PrimitiveValue : public Value {
				sybrenUnsubmitted Done Inline Actions Documentation, 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…
				private:
				T inner_value_;

				SeverinUnsubmitted Done Inline Actions Same code style fix needed for these other private variables 😛 Severin: Same code style fix needed for these other private variables 😛
				public:
				PrimitiveValue(const T value) : Value(V), inner_value_(value)
				{
				}

				const T value() const
				{
				return inner_value_;
				}
				};

				class NullValue : public Value {
				sybrenUnsubmitted Done Inline Actions Remove "Helper function to" sybren: Remove "Helper function to"
				public:
				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
				{
				sybrenUnsubmitted Done Inline Actions Remove "Helper class for" sybren: Remove "Helper class for"
				return inner_value_;
				}

				Container &elements()
				{
				sybrenUnsubmitted Done Inline Actions Instead 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…
				return inner_value_;
				}
				};

				/* Internal storage type for ObjectValue. */
				using ObjectElementType = std::pair<std::string, std::shared_ptr<Value>>;

				/**
				* Object is a key value container where the key must by a std::string.
				sybrenUnsubmitted Done Inline Actions be sybren: be
				sybrenUnsubmitted Done Inline Actions key-value sybren: key-value
				* Internally it is stored in a blender::Vector to ensure the order of keys.
				SeverinUnsubmitted Done Inline Actions Better 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…
				*/
				class ObjectValue
				SeverinUnsubmitted Done Inline Actions This 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…
				: public ContainerValue<Vector<ObjectElementType>, ObjectElementType, eValueType::Object> {
				SeverinUnsubmitted Done Inline Actions Think this should be `explicit`. Severin: Think this should be `explicit`.
				public:
				using LookupValue = std::shared_ptr<Value>;
				sybrenUnsubmitted Done Inline Actions Why 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?
				using Lookup = Map<std::string, LookupValue>;

				/** Return a lookup map to quickly lookup by key. */
				const Lookup create_lookup() const
				{
				Lookup result;
				for (const Item &item : elements()) {
				result.add_as(item.first, item.second);
				}
				return result;
				}
				};

				class Formatter {
				SeverinUnsubmitted Done Inline Actions There 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.
				sybrenUnsubmitted Done Inline Actions Document sybren: Document
				public:
				virtual ~Formatter() = default;
				virtual void serialize(std::ostream &os, Value &value) = 0;
				sybrenUnsubmitted Done Inline Actions Why 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…
				virtual std::unique_ptr<Value> deserialize(std::istream &is) = 0;
				};

				class JsonFormatter : public Formatter {
				sybrenUnsubmitted Done Inline Actions Document sybren: Document
				public:
				uint8_t indentation_len = 0;
				sybrenUnsubmitted Done Inline Actions Unless 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…
				sybrenUnsubmitted Done Inline Actions This 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…

				public:
				void serialize(std::ostream &os, Value &value) override;
				std::unique_ptr<Value> deserialize(std::istream &is) override;
				};

				} // namespace blender::io::serialize
				SeverinUnsubmitted Not Done Inline Actions This 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…
				jbakkerAuthorUnsubmitted Done Inline Actions Yes I considered it. Goal was a minialistic abstration layer. I agree that the public API is much cleaner. Main reason for the current API is to control the performance hit when it is actually being used and reduction of overhead. 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…
				jbakkerAuthorUnsubmitted Done Inline Actions JSON specs are also not 100% clear about duplicated keys inside a JSON file. It should be, but you are allowed to parse or add them. jbakker: JSON specs are also not 100% clear about duplicated keys inside a JSON file. It should be, but…
				SeverinUnsubmitted Done Inline Actions Very 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…