birkeh/o3de
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Address some review feedback

Browse Source 
Signed-off-by: Nicholas Van Sickle <nvsickle@amazon.com>
This commit is contained in:
Nicholas Van Sickle
2022-01-04 17:41:58 -08:00
parent e518faa8be
commit 41c0fb2b02
7 changed files with 203 additions and 141 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Code/Framework/AzCore/AzCore/DOM/DomUtils.cpp
+1 -1
View File
@@ -22,7 +22,7 @@ namespace AZ::Dom::Utils
return backend.ReadFromBufferInPlace(string.data(), string.size(), visitor);
}
AZ::Outcome<Value, AZStd::string> WriteToValue(Backend::WriteCallback writeCallback)
AZ::Outcome<Value, AZStd::string> WriteToValue(const Backend::WriteCallback& writeCallback)
{
Value value;
AZStd::unique_ptr<Visitor> writer = value.GetWriteHandler();
Code/Framework/AzCore/AzCore/DOM/DomUtils.h
+1 -1
View File
@@ -16,5 +16,5 @@ namespace AZ::Dom::Utils
Visitor::Result ReadFromString(Backend& backend, AZStd::string_view string, AZ::Dom::Lifetime lifetime, Visitor& visitor);
Visitor::Result ReadFromStringInPlace(Backend& backend, AZStd::string& string, Visitor& visitor);
AZ::Outcome<Value, AZStd::string> WriteToValue(Backend::WriteCallback writeCallback);
AZ::Outcome<Value, AZStd::string> WriteToValue(const Backend::WriteCallback& writeCallback);
} // namespace AZ::Dom::Utils
Code/Framework/AzCore/AzCore/DOM/DomValue.cpp
+155 -93
View File
@@ -12,22 +12,22 @@
namespace AZ::Dom
{
template<class T>
AZStd::shared_ptr<T>& CheckCopyOnWrite(AZStd::shared_ptr<T>& refCountedPointer)
{
if (refCountedPointer.use_count() == 1)
{
return refCountedPointer;
}
else
{
refCountedPointer = AZStd::allocate_shared<T>(StdValueAllocator(), *refCountedPointer);
return refCountedPointer;
}
}
namespace Internal
{
template<class T>
AZStd::shared_ptr<T>& CheckCopyOnWrite(AZStd::shared_ptr<T>& refCountedPointer)
{
if (refCountedPointer.use_count() == 1)
{
return refCountedPointer;
}
else
{
refCountedPointer = AZStd::allocate_shared<T>(StdValueAllocator(), *refCountedPointer);
return refCountedPointer;
}
}
template<class TestType>
constexpr size_t GetTypeIndexInternal(size_t index = 0);
@@ -288,107 +288,173 @@ namespace AZ::Dom
Type Dom::Value::GetType() const
{
switch (m_value.index())
{
case GetTypeIndex<AZStd::monostate>():
return Type::Null;
case GetTypeIndex<int64_t>():
return Type::Int64;
case GetTypeIndex<uint64_t>():
return Type::Uint64;
case GetTypeIndex<double>():
return Type::Double;
case GetTypeIndex<bool>():
return Type::Bool;
case GetTypeIndex<AZStd::string_view>():
case GetTypeIndex<SharedStringType>():
case GetTypeIndex<ShortStringType>():
return Type::String;
case GetTypeIndex<ObjectPtr>():
return Type::Object;
case GetTypeIndex<ArrayPtr>():
return Type::Array;
case GetTypeIndex<NodePtr>():
return Type::Node;
case GetTypeIndex<AZStd::shared_ptr<AZStd::any>>():
return Type::Opaque;
}
AZ_Assert(false, "AZ::Dom::Value::GetType: m_value has an unexpected type");
return Type::Null;
return AZStd::visit(
[](auto&& value) -> Type
{
using CurrentType = AZStd::decay_t<decltype(value)>;
if constexpr (AZStd::is_same_v<CurrentType, AZStd::monostate>)
{
return Type::Null;
}
else if constexpr (AZStd::is_same_v<CurrentType, int64_t>)
{
return Type::Int64;
}
else if constexpr (AZStd::is_same_v<CurrentType, uint64_t>)
{
return Type::Uint64;
}
else if constexpr (AZStd::is_same_v<CurrentType, double>)
{
return Type::Double;
}
else if constexpr (AZStd::is_same_v<CurrentType, bool>)
{
return Type::Bool;
}
else if constexpr (AZStd::is_same_v<CurrentType, AZStd::string_view>)
{
return Type::String;
}
else if constexpr (AZStd::is_same_v<CurrentType, SharedStringType>)
{
return Type::String;
}
else if constexpr (AZStd::is_same_v<CurrentType, ShortStringType>)
{
return Type::String;
}
else if constexpr (AZStd::is_same_v<CurrentType, ObjectPtr>)
{
return Type::Object;
}
else if constexpr (AZStd::is_same_v<CurrentType, ArrayPtr>)
{
return Type::Array;
}
else if constexpr (AZStd::is_same_v<CurrentType, NodePtr>)
{
return Type::Node;
}
else if constexpr (AZStd::is_same_v<CurrentType, OpaqueStorageType>)
{
return Type::Opaque;
}
else
{
static_assert(false, "AZ::Dom::Value::GetType: m_value has an unexpected type");
}
},
m_value);
}
bool Value::IsNull() const
{
return GetType() == Type::Null;
return AZStd::holds_alternative<AZStd::monostate>(m_value);
}
bool Value::IsFalse() const
{
return IsBool() && !AZStd::get<bool>(m_value);
const bool* value = AZStd::get_if<bool>(&m_value);
return value != nullptr ? !(*value) : false;
}
bool Value::IsTrue() const
{
return IsBool() && AZStd::get<bool>(m_value);
const bool* value = AZStd::get_if<bool>(&m_value);
return value != nullptr ? *value : false;
}
bool Value::IsBool() const
{
return GetType() == Type::Bool;
return AZStd::holds_alternative<bool>(m_value);
}
bool Value::IsNode() const
{
return GetType() == Type::Node;
return AZStd::holds_alternative<NodePtr>(m_value);
}
bool Value::IsObject() const
{
return GetType() == Type::Object;
return AZStd::holds_alternative<ObjectPtr>(m_value);
}
bool Value::IsArray() const
{
return GetType() == Type::Array;
return AZStd::holds_alternative<ArrayPtr>(m_value);
}
bool Value::IsOpaqueValue() const
{
return GetType() == Type::Opaque;
return AZStd::holds_alternative<OpaqueStorageType>(m_value);
}
bool Value::IsNumber() const
{
switch (GetType())
{
case Type::Int64:
[[fallthrough]];
case Type::Uint64:
[[fallthrough]];
case Type::Double:
return true;
}
return false;
return AZStd::visit(
[](auto&& value) -> bool
{
using CurrentType = AZStd::decay_t<decltype(value)>;
if constexpr (AZStd::is_same_v<CurrentType, int64_t>)
{
return true;
}
else if constexpr (AZStd::is_same_v<CurrentType, uint64_t>)
{
return true;
}
else if constexpr (AZStd::is_same_v<CurrentType, double>)
{
return true;
}
else
{
return false;
}
},
m_value);
}
bool Value::IsInt() const
{
return GetType() == Type::Int64;
return AZStd::holds_alternative<int64_t>(m_value);
}
bool Value::IsUint() const
{
return GetType() == Type::Uint64;
return AZStd::holds_alternative<uint64_t>(m_value);
}
bool Value::IsDouble() const
{
return GetType() == Type::Double;
return AZStd::holds_alternative<double>(m_value);
}
bool Value::IsString() const
{
return GetType() == Type::String;
return AZStd::visit(
[](auto&& value) -> bool
{
using CurrentType = AZStd::decay_t<decltype(value)>;
if constexpr (AZStd::is_same_v<CurrentType, AZStd::string_view>)
{
return true;
}
else if constexpr (AZStd::is_same_v<CurrentType, SharedStringType>)
{
return true;
}
else if constexpr (AZStd::is_same_v<CurrentType, ShortStringType>)
{
return true;
}
else
{
return false;
}
},
m_value);
}
Value& Value::SetObject()
@@ -406,7 +472,7 @@ namespace AZ::Dom
Node& Value::GetNodeInternal()
{
AZ_Assert(GetType() == Type::Node, "AZ::Dom::Value: attempted to retrieve a node from a non-node value");
return *CheckCopyOnWrite(AZStd::get<NodePtr>(m_value));
return *Internal::CheckCopyOnWrite(AZStd::get<NodePtr>(m_value));
}
const Object::ContainerType& Value::GetObjectInternal() const
@@ -433,11 +499,11 @@ namespace AZ::Dom
"AZ::Dom::Value: attempted to retrieve an object from a value that isn't an object or a node");
if (type == Type::Object)
{
return CheckCopyOnWrite(AZStd::get<ObjectPtr>(m_value))->m_values;
return Internal::CheckCopyOnWrite(AZStd::get<ObjectPtr>(m_value))->m_values;
}
else
{
return CheckCopyOnWrite(AZStd::get<NodePtr>(m_value))->GetProperties();
return Internal::CheckCopyOnWrite(AZStd::get<NodePtr>(m_value))->GetProperties();
}
}
@@ -465,11 +531,11 @@ namespace AZ::Dom
"AZ::Dom::Value: attempted to retrieve an array from a value that isn't an array or node");
if (type == Type::Array)
{
return CheckCopyOnWrite(AZStd::get<ArrayPtr>(m_value))->m_values;
return Internal::CheckCopyOnWrite(AZStd::get<ArrayPtr>(m_value))->m_values;
}
else
{
return CheckCopyOnWrite(AZStd::get<NodePtr>(m_value))->GetChildren();
return Internal::CheckCopyOnWrite(AZStd::get<NodePtr>(m_value))->GetChildren();
}
}
@@ -698,22 +764,22 @@ namespace AZ::Dom
return *this;
}
size_t Value::Size() const
size_t Value::ArraySize() const
{
return GetArrayInternal().size();
}
size_t Value::Capacity() const
size_t Value::ArrayCapacity() const
{
return GetArrayInternal().capacity();
}
bool Value::Empty() const
bool Value::IsArrayEmpty() const
{
return GetArrayInternal().empty();
}
void Value::Clear()
void Value::ClearArray()
{
GetArrayInternal().clear();
}
@@ -728,43 +794,43 @@ namespace AZ::Dom
return GetArrayInternal()[index];
}
Value& Value::MutableAt(size_t index)
Value& Value::MutableArrayAt(size_t index)
{
return operator[](index);
}
const Value& Value::At(size_t index) const
const Value& Value::ArrayAt(size_t index) const
{
return operator[](index);
}
Array::ConstIterator Value::Begin() const
Array::ConstIterator Value::ArrayBegin() const
{
return GetArrayInternal().begin();
}
Array::ConstIterator Value::End() const
Array::ConstIterator Value::ArrayEnd() const
{
return GetArrayInternal().end();
}
Array::Iterator Value::Begin()
Array::Iterator Value::ArrayBegin()
{
return GetArrayInternal().begin();
}
Array::Iterator Value::End()
Array::Iterator Value::ArrayEnd()
{
return GetArrayInternal().end();
}
Value& Value::Reserve(size_t newCapacity)
Value& Value::ArrayReserve(size_t newCapacity)
{
GetArrayInternal().reserve(newCapacity);
return *this;
}
Value& Value::PushBack(Value value)
Value& Value::ArrayPushBack(Value value)
{
Array::ContainerType& array = GetArrayInternal();
array.reserve((array.size() / Array::ReserveIncrement + 1) * Array::ReserveIncrement);
@@ -772,18 +838,18 @@ namespace AZ::Dom
return *this;
}
Value& Value::PopBack()
Value& Value::ArrayPopBack()
{
GetArrayInternal().pop_back();
return *this;
}
Array::Iterator Value::Erase(Array::ConstIterator pos)
Array::Iterator Value::ArrayErase(Array::ConstIterator pos)
{
return GetArrayInternal().erase(pos);
}
Array::Iterator Value::Erase(Array::ConstIterator first, Array::ConstIterator last)
Array::Iterator Value::ArrayErase(Array::ConstIterator first, Array::ConstIterator last)
{
return GetArrayInternal().erase(first, last);
}
@@ -1007,13 +1073,6 @@ namespace AZ::Dom
void Value::SetString(AZStd::string_view value)
{
if (value.size() <= ShortStringSize)
{
ShortStringType buffer;
buffer.resize_no_construct(value.size());
memcpy(buffer.data(), value.data(), value.size());
m_value = buffer;
}
m_value = value;
}
@@ -1021,7 +1080,10 @@ namespace AZ::Dom
{
if (value.size() <= ShortStringSize)
{
SetString(value);
ShortStringType buffer;
buffer.resize_no_construct(value.size());
memcpy(buffer.data(), value.data(), value.size());
m_value = buffer;
}
else
{
@@ -1035,9 +1097,9 @@ namespace AZ::Dom
return *AZStd::get<OpaqueStorageType>(m_value);
}
void Value::SetOpaqueValue(const AZStd::any& value)
void Value::SetOpaqueValue(AZStd::any value)
{
m_value = AZStd::allocate_shared<AZStd::any>(StdValueAllocator(), value);
m_value = AZStd::allocate_shared<AZStd::any>(StdValueAllocator(), AZStd::move(value));
}
void Value::SetNull()
Code/Framework/AzCore/AzCore/DOM/DomValue.h
+19 -19
View File
@@ -103,9 +103,9 @@ namespace AZ::Dom
{
public:
Node() = default;
Node(const Node&) = default;
Node(Node&&) = default;
explicit Node(AZ::Name name);
explicit Node(const Node&) = default;
explicit Node(Node&&) = default;
Node& operator=(const Node&) = default;
Node& operator=(Node&&) = default;
@@ -174,7 +174,7 @@ namespace AZ::Dom
double,
// Bool
bool,
// StringType
// String
AZStd::string_view,
SharedStringType,
ShortStringType,
@@ -280,28 +280,28 @@ namespace AZ::Dom
// Array API (also used by Node)...
Value& SetArray();
size_t Size() const;
size_t Capacity() const;
bool Empty() const;
void Clear();
size_t ArraySize() const;
size_t ArrayCapacity() const;
bool IsArrayEmpty() const;
void ClearArray();
Value& operator[](size_t index);
const Value& operator[](size_t index) const;
Value& MutableAt(size_t index);
const Value& At(size_t index) const;
Value& MutableArrayAt(size_t index);
const Value& ArrayAt(size_t index) const;
Array::ConstIterator Begin() const;
Array::ConstIterator End() const;
Array::Iterator Begin();
Array::Iterator End();
Array::ConstIterator ArrayBegin() const;
Array::ConstIterator ArrayEnd() const;
Array::Iterator ArrayBegin();
Array::Iterator ArrayEnd();
Value& Reserve(size_t newCapacity);
Value& PushBack(Value value);
Value& PopBack();
Value& ArrayReserve(size_t newCapacity);
Value& ArrayPushBack(Value value);
Value& ArrayPopBack();
Array::Iterator Erase(Array::ConstIterator pos);
Array::Iterator Erase(Array::ConstIterator first, Array::ConstIterator last);
Array::Iterator ArrayErase(Array::ConstIterator pos);
Array::Iterator ArrayErase(Array::ConstIterator first, Array::ConstIterator last);
Array::ContainerType& GetMutableArray();
const Array::ContainerType& GetArray() const;
@@ -365,7 +365,7 @@ namespace AZ::Dom
//! serialize an opaque type into a DOM value instead, as serializers
//! and other systems will have no means of dealing with fully arbitrary
//! values.
void SetOpaqueValue(const AZStd::any&);
void SetOpaqueValue(AZStd::any);
// Null API...
void SetNull();
Code/Framework/AzCore/AzCore/DOM/DomValueWriter.cpp
+2 -2
View File
@@ -129,7 +129,7 @@ namespace AZ::Dom
AZStd::string::format("AZ::Dom::ValueWriter: %s called from within a different container type", endMethodName));
}
if (static_cast<AZ::u64>(buffer.m_attributes.size()) != attributeCount)
if (aznumeric_cast<AZ::u64>(buffer.m_attributes.size()) != attributeCount)
{
return VisitorFailure(
VisitorErrorCode::InternalError,
@@ -138,7 +138,7 @@ namespace AZ::Dom
buffer.m_attributes.size()));
}
if (static_cast<AZ::u64>(buffer.m_elements.size()) != elementCount)
if (aznumeric_cast<AZ::u64>(buffer.m_elements.size()) != elementCount)
{
return VisitorFailure(
VisitorErrorCode::InternalError,
Code/Framework/AzCore/Tests/DOM/DomValueBenchmarks.cpp
+2 -2
View File
@@ -77,7 +77,7 @@ namespace AZ::Dom::Benchmark
Value array(Type::Array);
for (int i = 0; i < entryCount; ++i)
{
array.PushBack(createEntry(i));
array.ArrayPushBack(createEntry(i));
}
return array;
};
@@ -152,7 +152,7 @@ namespace AZ::Dom::Benchmark
for (auto _ : state)
{
Value copy = original;
copy["entries"]["Key0"].PushBack(42);
copy["entries"]["Key0"].ArrayPushBack(42);
TakeAndDiscardWithoutTimingDtor(AZStd::move(copy), state);
}
Code/Framework/AzCore/Tests/DOM/DomValueTests.cpp
+23 -23
View File
@@ -55,7 +55,7 @@ namespace AZ::Dom::Tests
m_value.SetArray();
EXPECT_TRUE(m_value.IsArray());
EXPECT_EQ(m_value.Size(), 0);
EXPECT_EQ(m_value.ArraySize(), 0);
PerformValueChecks();
}
@@ -66,8 +66,8 @@ namespace AZ::Dom::Tests
for (int i = 0; i < 5; ++i)
{
m_value.PushBack(Value(i));
EXPECT_EQ(m_value.Size(), i + 1);
m_value.ArrayPushBack(Value(i));
EXPECT_EQ(m_value.ArraySize(), i + 1);
EXPECT_EQ(m_value[i].GetInt32(), i);
}
@@ -82,15 +82,15 @@ namespace AZ::Dom::Tests
Value nestedArray(Type::Array);
for (int i = 0; i < 5; ++i)
{
nestedArray.PushBack(Value(i));
nestedArray.ArrayPushBack(Value(i));
}
m_value.PushBack(AZStd::move(nestedArray));
m_value.ArrayPushBack(AZStd::move(nestedArray));
}
EXPECT_EQ(m_value.Size(), 5);
EXPECT_EQ(m_value.ArraySize(), 5);
for (int i = 0; i < 3; ++i)
{
EXPECT_EQ(m_value[i].Size(), 5);
EXPECT_EQ(m_value[i].ArraySize(), 5);
for (int j = 0; j < 5; ++j)
{
EXPECT_EQ(m_value[i][j].GetInt32(), j);
@@ -154,7 +154,7 @@ namespace AZ::Dom::Tests
m_value.SetNode("Test");
EXPECT_EQ(m_value.GetNodeName(), AZ::Name("Test"));
EXPECT_EQ(m_value.MemberCount(), 0);
EXPECT_EQ(m_value.Size(), 0);
EXPECT_EQ(m_value.ArraySize(), 0);
PerformValueChecks();
}
@@ -165,8 +165,8 @@ namespace AZ::Dom::Tests
for (int i = 0; i < 10; ++i)
{
m_value.PushBack(Value(i));
EXPECT_EQ(m_value.Size(), i + 1);
m_value.ArrayPushBack(Value(i));
EXPECT_EQ(m_value.ArraySize(), i + 1);
EXPECT_EQ(m_value[i].GetInt32(), i);
if (i < 5)
@@ -196,10 +196,10 @@ namespace AZ::Dom::Tests
childNode.AddMember("foo", i);
childNode.AddMember("bar", Value("test", false));
m_value.PushBack(childNode);
m_value.ArrayPushBack(childNode);
}
EXPECT_EQ(m_value.Size(), 5);
EXPECT_EQ(m_value.ArraySize(), 5);
for (int i = 0; i < 5; ++i)
{
const Value& childNode = m_value[i];
@@ -334,30 +334,30 @@ namespace AZ::Dom::Tests
TEST_F(DomValueTests, CopyOnWrite_Array)
{
Value v1(Type::Array);
v1.PushBack(1);
v1.PushBack(2);
v1.ArrayPushBack(1);
v1.ArrayPushBack(2);
Value nestedArray(Type::Array);
v1.PushBack(nestedArray);
v1.ArrayPushBack(nestedArray);
Value v2 = v1;
EXPECT_EQ(&v1.GetArray(), &v2.GetArray());
EXPECT_EQ(&v1.At(2).GetArray(), &v2.At(2).GetArray());
EXPECT_EQ(&v1.ArrayAt(2).GetArray(), &v2.ArrayAt(2).GetArray());
v2[0] = 0;
EXPECT_NE(&v1.GetArray(), &v2.GetArray());
EXPECT_EQ(&v1.At(2).GetArray(), &v2.At(2).GetArray());
EXPECT_EQ(&v1.ArrayAt(2).GetArray(), &v2.ArrayAt(2).GetArray());
v2[2].PushBack(42);
v2[2].ArrayPushBack(42);
EXPECT_NE(&v1.GetArray(), &v2.GetArray());
EXPECT_NE(&v1.At(2).GetArray(), &v2.At(2).GetArray());
EXPECT_NE(&v1.ArrayAt(2).GetArray(), &v2.ArrayAt(2).GetArray());
v2 = v1;
EXPECT_EQ(&v1.GetArray(), &v2.GetArray());
EXPECT_EQ(&v1.At(2).GetArray(), &v2.At(2).GetArray());
EXPECT_EQ(&v1.ArrayAt(2).GetArray(), &v2.ArrayAt(2).GetArray());
}
TEST_F(DomValueTests, CopyOnWrite_Node)
@@ -365,8 +365,8 @@ namespace AZ::Dom::Tests
Value v1;
v1.SetNode("TopLevel");
v1.PushBack(1);
v1.PushBack(2);
v1.ArrayPushBack(1);
v1.ArrayPushBack(2);
v1["obj"].SetNode("Nested");
Value v2 = v1;
@@ -378,7 +378,7 @@ namespace AZ::Dom::Tests
EXPECT_NE(&v1.GetNode(), &v2.GetNode());
EXPECT_EQ(&v1["obj"].GetNode(), &v2["obj"].GetNode());
v2["obj"].PushBack(42);
v2["obj"].ArrayPushBack(42);
EXPECT_NE(&v1.GetNode(), &v2.GetNode());
EXPECT_NE(&v1["obj"].GetNode(), &v2["obj"].GetNode());