Skip to content

Commit

Permalink
Revert "[glass] Storage: Store Value by value"
Browse files Browse the repository at this point in the history 
This reverts commit 309b370.

Storage::GetChildArray never initializes the child array when the entry is new, so it returns invalid references.
  • Loading branch information
@rzblue
rzblue committed Nov 4, 2024
1 parent debb521 commit c23559b
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 98 deletions.
158 changes: 94 additions & 64 deletions glass/src/lib/native/cpp/Storage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,15 @@ Storage::Value* Storage::FindValue(std::string_view key) {
if (it == m_values.end()) {
return nullptr;
}
return &it->second;
return it->second.get();
}

Storage::Value& Storage::GetValue(std::string_view key) {
auto& val = m_values[key];
if (!val) {
val = std::make_unique<Value>();
}
return *val;
}

#define DEFUN(CapsName, LowerName, CType, CParamType, ArrCType) \
Expand All @@ -208,7 +216,7 @@ Storage::Value* Storage::FindValue(std::string_view key) {
if (it == m_values.end()) { \
return CType{defaultVal}; \
} \
Value& value = it->second; \
Value& value = *it->second; \
if (value.type != Value::k##CapsName) { \
if (!Convert##CapsName(&value)) { \
value.Reset(Value::k##CapsName); \
Expand All @@ -221,59 +229,66 @@ Storage::Value* Storage::FindValue(std::string_view key) {
} \
\
void Storage::Set##CapsName(std::string_view key, CParamType val) { \
auto [it, isNew] = m_values.try_emplace(key, Value::k##CapsName); \
if (!isNew) { \
it->second.Reset(Value::k##CapsName); \
auto& valuePtr = m_values[key]; \
if (!valuePtr) { \
valuePtr = std::make_unique<Value>(Value::k##CapsName); \
} else { \
valuePtr->Reset(Value::k##CapsName); \
} \
it->second.LowerName##Val = val; \
it->second.LowerName##Default = {}; \
valuePtr->LowerName##Val = val; \
valuePtr->LowerName##Default = {}; \
} \
\
CType& Storage::Get##CapsName(std::string_view key, CParamType defaultVal) { \
auto [it, setValue] = m_values.try_emplace(key, Value::k##CapsName); \
if (!setValue && it->second.type != Value::k##CapsName) { \
if (!Convert##CapsName(&it->second)) { \
it->second.Reset(Value::k##CapsName); \
auto& valuePtr = m_values[key]; \
bool setValue = false; \
if (!valuePtr) { \
valuePtr = std::make_unique<Value>(Value::k##CapsName); \
setValue = true; \
} else if (valuePtr->type != Value::k##CapsName) { \
if (!Convert##CapsName(valuePtr.get())) { \
valuePtr->Reset(Value::k##CapsName); \
setValue = true; \
} \
} \
Value& value = it->second; \
if (setValue) { \
value.LowerName##Val = defaultVal; \
valuePtr->LowerName##Val = defaultVal; \
} \
if (!value.hasDefault) { \
value.LowerName##Default = defaultVal; \
value.hasDefault = true; \
if (!valuePtr->hasDefault) { \
valuePtr->LowerName##Default = defaultVal; \
valuePtr->hasDefault = true; \
} \
return value.LowerName##Val; \
return valuePtr->LowerName##Val; \
} \
\
std::vector<ArrCType>& Storage::Get##CapsName##Array( \
std::string_view key, std::span<const ArrCType> defaultVal) { \
auto [it, setValue] = \
m_values.try_emplace(key, Value::k##CapsName##Array); \
if (!setValue && it->second.type != Value::k##CapsName##Array) { \
if (!Convert##CapsName##Array(&it->second)) { \
it->second.Reset(Value::k##CapsName##Array); \
auto& valuePtr = m_values[key]; \
bool setValue = false; \
if (!valuePtr) { \
valuePtr = std::make_unique<Value>(Value::k##CapsName##Array); \
setValue = true; \
} else if (valuePtr->type != Value::k##CapsName##Array) { \
if (!Convert##CapsName##Array(valuePtr.get())) { \
valuePtr->Reset(Value::k##CapsName##Array); \
setValue = true; \
} \
} \
Value& value = it->second; \
if (setValue) { \
value.LowerName##Array = \
valuePtr->LowerName##Array = \
new std::vector<ArrCType>{defaultVal.begin(), defaultVal.end()}; \
} \
if (!value.hasDefault) { \
if (!valuePtr->hasDefault) { \
if (defaultVal.empty()) { \
value.LowerName##ArrayDefault = nullptr; \
valuePtr->LowerName##ArrayDefault = nullptr; \
} else { \
value.LowerName##ArrayDefault = \
valuePtr->LowerName##ArrayDefault = \
new std::vector<ArrCType>{defaultVal.begin(), defaultVal.end()}; \
} \
value.hasDefault = true; \
valuePtr->hasDefault = true; \
} \
assert(value.LowerName##Array); \
return *value.LowerName##Array; \
assert(valuePtr->LowerName##Array); \
return *valuePtr->LowerName##Array; \
}

DEFUN(Int, int, int, int, int)
Expand All @@ -288,35 +303,45 @@ Storage& Storage::GetChild(std::string_view label_id) {
if (id.empty()) {
id = label;
}
Value& childValue = GetValue(id);
if (childValue.type != Value::kChild) {
childValue.Reset(Value::kChild);
childValue.child = new Storage;
auto& childPtr = m_values[id];
if (!childPtr) {
childPtr = std::make_unique<Value>();
}
if (childPtr->type != Value::kChild) {
childPtr->Reset(Value::kChild);
childPtr->child = new Storage;
}
return *childValue.child;
return *childPtr->child;
}

std::vector<std::unique_ptr<Storage>>& Storage::GetChildArray(
std::string_view key) {
auto [it, isNew] = m_values.try_emplace(key, Value::kChildArray);
if (!isNew && it->second.type != Value::kChildArray) {
it->second.Reset(Value::kChildArray);
it->second.childArray = new std::vector<std::unique_ptr<Storage>>();
auto& valuePtr = m_values[key];
if (!valuePtr) {
valuePtr = std::make_unique<Value>(Value::kChildArray);
valuePtr->childArray = new std::vector<std::unique_ptr<Storage>>();
} else if (valuePtr->type != Value::kChildArray) {
valuePtr->Reset(Value::kChildArray);
valuePtr->childArray = new std::vector<std::unique_ptr<Storage>>();
}

return *it->second.childArray;
return *valuePtr->childArray;
}

void Storage::Erase(std::string_view key) {
std::unique_ptr<Storage::Value> Storage::Erase(std::string_view key) {
auto it = m_values.find(key);
if (it != m_values.end()) {
auto rv = std::move(it->second);
m_values.erase(it);
return rv;
}
return nullptr;
}

void Storage::EraseChildren() {
std::erase_if(m_values,
[](const auto& kv) { return kv.second.type == Value::kChild; });
std::erase_if(m_values, [](const auto& kv) {
return kv.second->type == Value::kChild;
});
}

static bool JsonArrayToStorage(Storage::Value* valuePtr, const wpi::json& jarr,
Expand Down Expand Up @@ -448,47 +473,52 @@ bool Storage::FromJson(const wpi::json& json, const char* filename) {
return false;
}
for (auto&& jkv : json.items()) {
auto [it, created] = m_values.try_emplace(jkv.key());
auto& valuePtr = m_values[jkv.key()];
bool created = false;
if (!valuePtr) {
valuePtr = std::make_unique<Value>();
created = true;
}
auto& jvalue = jkv.value();
switch (jvalue.type()) {
case wpi::json::value_t::boolean:
it->second.Reset(Value::kBool);
it->second.boolVal = jvalue.get<bool>();
valuePtr->Reset(Value::kBool);
valuePtr->boolVal = jvalue.get<bool>();
break;
case wpi::json::value_t::number_float:
it->second.Reset(Value::kDouble);
it->second.doubleVal = jvalue.get<double>();
valuePtr->Reset(Value::kDouble);
valuePtr->doubleVal = jvalue.get<double>();
break;
case wpi::json::value_t::number_integer:
it->second.Reset(Value::kInt64);
it->second.int64Val = jvalue.get<int64_t>();
valuePtr->Reset(Value::kInt64);
valuePtr->int64Val = jvalue.get<int64_t>();
break;
case wpi::json::value_t::number_unsigned:
it->second.Reset(Value::kInt64);
it->second.int64Val = jvalue.get<uint64_t>();
valuePtr->Reset(Value::kInt64);
valuePtr->int64Val = jvalue.get<uint64_t>();
break;
case wpi::json::value_t::string:
it->second.Reset(Value::kString);
it->second.stringVal = jvalue.get_ref<const std::string&>();
valuePtr->Reset(Value::kString);
valuePtr->stringVal = jvalue.get_ref<const std::string&>();
break;
case wpi::json::value_t::object:
if (it->second.type != Value::kChild) {
it->second.Reset(Value::kChild);
it->second.child = new Storage;
if (valuePtr->type != Value::kChild) {
valuePtr->Reset(Value::kChild);
valuePtr->child = new Storage;
}
it->second.child->FromJson(jvalue, filename); // recurse
valuePtr->child->FromJson(jvalue, filename); // recurse
break;
case wpi::json::value_t::array:
if (!JsonArrayToStorage(&it->second, jvalue, filename)) {
if (!JsonArrayToStorage(valuePtr.get(), jvalue, filename)) {
if (created) {
m_values.erase(it);
m_values.erase(jkv.key());
}
}
break;
default:
ImGui::LogText("null value in %s, ignoring", filename);
if (created) {
m_values.erase(it);
m_values.erase(jkv.key());
}
break;
}
Expand Down Expand Up @@ -527,7 +557,7 @@ wpi::json Storage::ToJson() const {
wpi::json j = wpi::json::object();
for (auto&& kv : m_values) {
wpi::json jelem;
Value& value = kv.second;
auto& value = *kv.second;
switch (value.type) {
#define CASE(CapsName, LowerName) \
case Value::k##CapsName: \
Expand Down Expand Up @@ -585,7 +615,7 @@ void Storage::Clear() {

void Storage::ClearValues() {
for (auto&& kv : m_values) {
Value& value = kv.second;
auto& value = *kv.second;
switch (value.type) {
case Value::kInt:
value.intVal = value.intDefault;
Expand Down Expand Up @@ -671,7 +701,7 @@ void Storage::Apply() {

void Storage::ApplyChildren() {
for (auto&& kv : m_values) {
Value& value = kv.second;
auto& value = *kv.second;
switch (value.type) {
case Value::kChild:
value.child->Apply();
Expand Down
42 changes: 8 additions & 34 deletions glass/src/lib/native/include/glass/Storage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,22 +62,6 @@ class Storage {
explicit Value(Type type) : type{type} {}
Value(const Value&) = delete;
Value& operator=(const Value&) = delete;
Value(Value&& rhs)
: type{rhs.type},
stringVal{std::move(rhs.stringVal)},
stringDefault{std::move(rhs.stringDefault)},
hasDefault{rhs.hasDefault} {
rhs.type = kNone;
}
Value& operator=(Value&& rhs) {
Reset(kNone);
type = rhs.type;
stringVal = std::move(rhs.stringVal);
stringDefault = std::move(rhs.stringDefault);
hasDefault = rhs.hasDefault;
rhs.type = kNone;
return *this;
}
~Value() { Reset(kNone); }

Type type = kNone;
Expand Down Expand Up @@ -119,7 +103,7 @@ class Storage {
void Reset(Type newType);
};

using ValueMap = wpi::StringMap<Value>;
using ValueMap = wpi::StringMap<std::unique_ptr<Value>>;
template <typename Iterator>
using ChildIterator = detail::ChildIterator<Iterator>;

Expand Down Expand Up @@ -164,9 +148,7 @@ class Storage {
std::vector<std::unique_ptr<Storage>>& GetChildArray(std::string_view key);

Value* FindValue(std::string_view key);
Value& GetValue(std::string_view key) {
return m_values.try_emplace(key, Value::kNone).first->second;
}
Value& GetValue(std::string_view key);
Storage& GetChild(std::string_view label_id);

void SetData(std::shared_ptr<void>&& data) { m_data = std::move(data); }
Expand All @@ -176,23 +158,15 @@ class Storage {
return static_cast<T*>(m_data.get());
}

template <typename T, typename... Args>
T& GetOrNewData(Args&&... args) {
if (!m_data) {
m_data = std::make_shared<T>(std::forward<Args>(args)...);
}
return *static_cast<T*>(m_data.get());
}

Storage() = default;
Storage(const Storage&) = delete;
Storage& operator=(const Storage&) = delete;

void Insert(std::string_view key, Value&& value) {
m_values.try_emplace(std::string{key}, std::move(value));
void Insert(std::string_view key, std::unique_ptr<Value> value) {
m_values[key] = std::move(value);
}

void Erase(std::string_view key);
std::unique_ptr<Value> Erase(std::string_view key);

void EraseAll() { m_values.clear(); }

Expand Down Expand Up @@ -275,7 +249,7 @@ class ChildIterator {
public:
ChildIterator(IteratorType it, IteratorType end) noexcept
: anchor(it), end(end) {
while (anchor != end && anchor->second.type != Storage::Value::kChild) {
while (anchor != end && anchor->second->type != Storage::Value::kChild) {
++anchor;
}
}
Expand All @@ -286,7 +260,7 @@ class ChildIterator {
/// increment operator (needed for range-based for)
ChildIterator& operator++() {
++anchor;
while (anchor != end && anchor->second.type != Storage::Value::kChild) {
while (anchor != end && anchor->second->type != Storage::Value::kChild) {
++anchor;
}
return *this;
Expand All @@ -301,7 +275,7 @@ class ChildIterator {
std::string_view key() const { return anchor->first; }

/// return value of the iterator
Storage& value() const { return *anchor->second.child; }
Storage& value() const { return *anchor->second->child; }
};

} // namespace detail
Expand Down

0 comments on commit c23559b

Please sign in to comment.