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

Address review feedback

Browse Source 
Signed-off-by: Nicholas Van Sickle <nvsickle@amazon.com>
This commit is contained in:
Nicholas Van Sickle
2022-01-27 13:15:30 -08:00
parent e06b8782cc
commit 73f166241e
6 changed files with 143 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Code/Framework/AzCore/AzCore/DOM/DomPatch.cpp
+55 -46
View File
@@ -8,8 +8,8 @@
#include <AzCore/DOM/DomPatch.h>
#include <AzCore/DOM/DomUtils.h>
#include <AzCore/std/containers/unordered_set.h>
#include <AzCore/std/containers/queue.h>
#include <AzCore/std/containers/unordered_set.h>
namespace AZ::Dom
{
@@ -257,7 +257,8 @@ namespace AZ::Dom
return AZ::Failure(sourceLoad.TakeError());
}
return AZ::Success(PatchOperation::CopyOperation(Path(destLoad.GetValue().GetString()), Path(sourceLoad.GetValue().GetString())));
return AZ::Success(
PatchOperation::CopyOperation(Path(destLoad.GetValue().GetString()), Path(sourceLoad.GetValue().GetString())));
}
else if (op == "move")
{
@@ -272,7 +273,8 @@ namespace AZ::Dom
return AZ::Failure(sourceLoad.TakeError());
}
return AZ::Success(PatchOperation::MoveOperation(Path(destLoad.GetValue().GetString()), Path(sourceLoad.GetValue().GetString())));
return AZ::Success(
PatchOperation::MoveOperation(Path(destLoad.GetValue().GetString()), Path(sourceLoad.GetValue().GetString())));
}
else if (op == "test")
{
@@ -319,8 +321,9 @@ namespace AZ::Dom
const Value* existingValue = stateBeforeApplication.FindChild(m_domPath);
if (existingValue == nullptr)
{
return AZ::Failure(
AZStd::string::format("Unable to invert DOM remove patch, source path not found: %s", m_domPath.ToString().data()));
AZStd::string errorMessage = "Unable to invert DOM remove patch, source path not found: ";
m_domPath.AppendToString(errorMessage);
return AZ::Failure(AZStd::move(errorMessage));
}
return AZ::Success(PatchOperation::AddOperation(m_domPath, *existingValue));
}
@@ -330,8 +333,9 @@ namespace AZ::Dom
const Value* existingValue = stateBeforeApplication.FindChild(m_domPath);
if (existingValue == nullptr)
{
return AZ::Failure(AZStd::string::format(
"Unable to invert DOM replace patch, source path not found: %s", m_domPath.ToString().data()));
AZStd::string errorMessage = "Unable to invert DOM replace patch, source path not found: ";
m_domPath.AppendToString(errorMessage);
return AZ::Failure(AZStd::move(errorMessage));
}
return AZ::Success(PatchOperation::ReplaceOperation(m_domPath, *existingValue));
}
@@ -341,8 +345,9 @@ namespace AZ::Dom
const Value* existingValue = stateBeforeApplication.FindChild(m_domPath);
if (existingValue == nullptr)
{
return AZ::Failure(
AZStd::string::format("Unable to invert DOM copy patch, source path not found: %s", m_domPath.ToString().data()));
AZStd::string errorMessage = "Unable to invert DOM copy patch, source path not found: ";
m_domPath.AppendToString(errorMessage);
return AZ::Failure(AZStd::move(errorMessage));
}
return AZ::Success(PatchOperation::ReplaceOperation(m_domPath, *existingValue));
}
@@ -367,8 +372,9 @@ namespace AZ::Dom
const Value* existingValue = stateBeforeApplication.FindChild(commonAncestor);
if (existingValue == nullptr)
{
return AZ::Failure(AZStd::string::format(
"Unable to invert DOM move patch, common ancestor path not found: %s", commonAncestor.ToString().data()));
AZStd::string errorMessage = "Unable to invert DOM copy patch, common ancestor path not found: ";
commonAncestor.AppendToString(errorMessage);
return AZ::Failure(AZStd::move(errorMessage));
}
return AZ::Success(PatchOperation::ReplaceOperation(commonAncestor, *existingValue));
}
@@ -383,10 +389,10 @@ namespace AZ::Dom
}
AZ::Outcome<PatchOperation::PathContext, AZStd::string> PatchOperation::LookupPath(
Value& rootElement, const Path& path, AZ::u8 existenceCheckFlags)
Value& rootElement, const Path& path, ExistenceCheckFlags flags)
{
const bool verifyFullPath = existenceCheckFlags & VerifyFullPath;
const bool allowEndOfArray = existenceCheckFlags & AllowEndOfArray;
const bool verifyFullPath = (flags & ExistenceCheckFlags::VerifyFullPath) != ExistenceCheckFlags::DefaultExistenceCheck;
const bool allowEndOfArray = (flags & ExistenceCheckFlags::AllowEndOfArray) != ExistenceCheckFlags::DefaultExistenceCheck;
Path target = path;
if (target.Size() == 0)
@@ -414,7 +420,9 @@ namespace AZ::Dom
Value* targetValue = rootElement.FindMutableChild(target);
if (targetValue == nullptr)
{
return AZ::Failure(AZStd::string::format("Path not found (%s)", target.ToString().data()));
AZStd::string errorMessage = "Path not found: ";
target.AppendToString(errorMessage);
return AZ::Failure(AZStd::move(errorMessage));
}
if (destinationIndex.IsIndex() || destinationIndex.IsEndOfArray())
@@ -450,7 +458,7 @@ namespace AZ::Dom
PatchOperation::PatchOutcome PatchOperation::ApplyAdd(Value& rootElement) const
{
auto pathLookup = LookupPath(rootElement, m_domPath, AllowEndOfArray);
auto pathLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::AllowEndOfArray);
if (!pathLookup.IsSuccess())
{
return AZ::Failure(pathLookup.TakeError());
@@ -481,7 +489,7 @@ namespace AZ::Dom
PatchOperation::PatchOutcome PatchOperation::ApplyRemove(Value& rootElement) const
{
auto pathLookup = LookupPath(rootElement, m_domPath, VerifyFullPath | AllowEndOfArray);
auto pathLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::VerifyFullPath | ExistenceCheckFlags::AllowEndOfArray);
if (!pathLookup.IsSuccess())
{
return AZ::Failure(pathLookup.TakeError());
@@ -505,7 +513,7 @@ namespace AZ::Dom
PatchOperation::PatchOutcome PatchOperation::ApplyReplace(Value& rootElement) const
{
auto pathLookup = LookupPath(rootElement, m_domPath, VerifyFullPath);
auto pathLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::VerifyFullPath);
if (!pathLookup.IsSuccess())
{
return AZ::Failure(pathLookup.TakeError());
@@ -517,13 +525,13 @@ namespace AZ::Dom
PatchOperation::PatchOutcome PatchOperation::ApplyCopy(Value& rootElement) const
{
auto sourceLookup = LookupPath(rootElement, GetSourcePath(), VerifyFullPath);
auto sourceLookup = LookupPath(rootElement, GetSourcePath(), ExistenceCheckFlags::VerifyFullPath);
if (!sourceLookup.IsSuccess())
{
return AZ::Failure(sourceLookup.TakeError());
}
auto destLookup = LookupPath(rootElement, m_domPath, AllowEndOfArray);
auto destLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::AllowEndOfArray);
if (!destLookup.IsSuccess())
{
return AZ::Failure(destLookup.TakeError());
@@ -535,13 +543,13 @@ namespace AZ::Dom
PatchOperation::PatchOutcome PatchOperation::ApplyMove(Value& rootElement) const
{
auto sourceLookup = LookupPath(rootElement, GetSourcePath(), VerifyFullPath);
auto sourceLookup = LookupPath(rootElement, GetSourcePath(), ExistenceCheckFlags::VerifyFullPath);
if (!sourceLookup.IsSuccess())
{
return AZ::Failure(sourceLookup.TakeError());
}
auto destLookup = LookupPath(rootElement, m_domPath, AllowEndOfArray);
auto destLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::AllowEndOfArray);
if (!destLookup.IsSuccess())
{
return AZ::Failure(destLookup.TakeError());
@@ -568,7 +576,7 @@ namespace AZ::Dom
PatchOperation::PatchOutcome PatchOperation::ApplyTest(Value& rootElement) const
{
auto pathLookup = LookupPath(rootElement, m_domPath, VerifyFullPath);
auto pathLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::VerifyFullPath);
if (!pathLookup.IsSuccess())
{
return AZ::Failure(pathLookup.TakeError());
@@ -670,32 +678,32 @@ namespace AZ::Dom
return m_operations[index];
}
Patch::OperationsContainer::iterator Patch::begin()
auto Patch::begin() -> OperationsContainer::iterator
{
return m_operations.begin();
}
Patch::OperationsContainer::iterator Patch::end()
auto Patch::end() -> OperationsContainer::iterator
{
return m_operations.end();
}
Patch::OperationsContainer::const_iterator Patch::begin() const
auto Patch::begin() const -> OperationsContainer::const_iterator
{
return m_operations.cbegin();
}
Patch::OperationsContainer::const_iterator Patch::end() const
auto Patch::end() const -> OperationsContainer::const_iterator
{
return m_operations.cend();
}
Patch::OperationsContainer::const_iterator Patch::cbegin() const
auto Patch::cbegin() const -> OperationsContainer::const_iterator
{
return m_operations.cbegin();
}
Patch::OperationsContainer::const_iterator Patch::cend() const
auto Patch::cend() const -> OperationsContainer::const_iterator
{
return m_operations.cend();
}
@@ -794,7 +802,8 @@ namespace AZ::Dom
return PatchOperation(AZStd::move(testPath), PatchOperation::Type::Test, AZStd::move(value));
}
PatchInfo GenerateHierarchicalDeltaPatch(const Value& beforeState, const Value& afterState)
PatchInfo GenerateHierarchicalDeltaPatch(
const Value& beforeState, const Value& afterState, const DeltaPatchGenerationParameters& params)
{
PatchInfo patches;
@@ -804,7 +813,7 @@ namespace AZ::Dom
patches.m_inversePatches.PushFront(AZStd::move(inverse));
};
AZStd::function<void(const Path&, const Value&, const Value&)> CompareValues;
AZStd::function<void(const Path&, const Value&, const Value&)> compareValues;
struct PendingComparison
{
@@ -822,7 +831,7 @@ namespace AZ::Dom
AZStd::queue<PendingComparison> entriesToCompare;
AZStd::unordered_set<AZ::Name::Hash> desiredKeys;
auto CompareObjects = [&](const Path& path, const Value& before, const Value& after)
auto compareObjects = [&](const Path& path, const Value& before, const Value& after)
{
desiredKeys.clear();
Path subPath = path;
@@ -853,22 +862,22 @@ namespace AZ::Dom
}
};
auto CompareArrays = [&](const Path& path, const Value& before, const Value& after)
auto compareArrays = [&](const Path& path, const Value& before, const Value& after)
{
const size_t beforeSize = before.ArraySize();
const size_t afterSize = after.ArraySize();
// If more than replaceThreshold values differ, do a replace operation instead
constexpr size_t replaceThreshold = 3;
size_t changedValueCount = 0;
for (size_t i = 0; i < afterSize; ++i)
if (params.m_replaceThreshold != DeltaPatchGenerationParameters::NoReplace)
{
if (i < beforeSize)
size_t changedValueCount = 0;
const size_t entriesToEnumerate = AZStd::min(beforeSize, afterSize);
for (size_t i = 0; i < entriesToEnumerate; ++i)
{
if (before[i] != after[i])
{
++changedValueCount;
if (changedValueCount >= replaceThreshold)
if (changedValueCount >= params.m_replaceThreshold)
{
AddPatch(PatchOperation::ReplaceOperation(path, after), PatchOperation::ReplaceOperation(path, before));
return;
@@ -904,7 +913,7 @@ namespace AZ::Dom
}
};
auto CompareNodes = [&](const Path& path, const Value& before, const Value& after)
auto compareNodes = [&](const Path& path, const Value& before, const Value& after)
{
if (before.GetNodeName() != after.GetNodeName())
{
@@ -912,12 +921,12 @@ namespace AZ::Dom
}
else
{
CompareObjects(path, before, after);
CompareArrays(path, before, after);
compareObjects(path, before, after);
compareArrays(path, before, after);
}
};
CompareValues = [&](const Path& path, const Value& before, const Value& after)
compareValues = [&](const Path& path, const Value& before, const Value& after)
{
if (before.GetType() != after.GetType())
{
@@ -931,15 +940,15 @@ namespace AZ::Dom
}
else if (before.IsObject())
{
CompareObjects(path, before, after);
compareObjects(path, before, after);
}
else if (before.IsArray())
{
CompareArrays(path, before, after);
compareArrays(path, before, after);
}
else if (before.IsNode())
{
CompareNodes(path, before, after);
compareNodes(path, before, after);
}
else
{
@@ -951,7 +960,7 @@ namespace AZ::Dom
while (!entriesToCompare.empty())
{
PendingComparison& comparison = entriesToCompare.front();
CompareValues(comparison.m_path, comparison.m_before, comparison.m_after);
compareValues(comparison.m_path, comparison.m_before, comparison.m_after);
entriesToCompare.pop();
}
return patches;
Code/Framework/AzCore/AzCore/DOM/DomPatch.h
+25 -10
View File
@@ -35,9 +35,9 @@ namespace AZ::Dom
PatchOperation(const PatchOperation&) = default;
PatchOperation(PatchOperation&&) = default;
explicit PatchOperation(Path destinationPath, Type type, Value value);
explicit PatchOperation(Path destionationPath, Type type, Path sourcePath);
explicit PatchOperation(Path path, Type type);
PatchOperation(Path destionationPath, Type type, Value value);
PatchOperation(Path destionationPath, Type type, Path sourcePath);
PatchOperation(Path path, Type type);
static PatchOperation AddOperation(Path destinationPath, Value value);
static PatchOperation RemoveOperation(Path pathToRemove);
@@ -72,6 +72,13 @@ namespace AZ::Dom
AZ::Outcome<PatchOperation, AZStd::string> GetInverse(Value stateBeforeApplication) const;
enum class ExistenceCheckFlags : AZ::u8
{
DefaultExistenceCheck = 0x0,
VerifyFullPath = 0x1,
AllowEndOfArray = 0x2,
};
private:
struct PathContext
{
@@ -79,12 +86,8 @@ namespace AZ::Dom
PathEntry m_key;
};
static constexpr AZ::u8 DefaultExistenceCheck = 0x0;
static constexpr AZ::u8 VerifyFullPath = 0x1;
static constexpr AZ::u8 AllowEndOfArray = 0x2;
static AZ::Outcome<PathContext, AZStd::string> LookupPath(
Value& rootElement, const Path& path, AZ::u8 existenceCheckFlags = DefaultExistenceCheck);
Value& rootElement, const Path& path, ExistenceCheckFlags existenceCheckFlags = ExistenceCheckFlags::DefaultExistenceCheck);
PatchOutcome ApplyAdd(Value& rootElement) const;
PatchOutcome ApplyRemove(Value& rootElement) const;
@@ -93,11 +96,13 @@ namespace AZ::Dom
PatchOutcome ApplyMove(Value& rootElement) const;
PatchOutcome ApplyTest(Value& rootElement) const;
AZStd::variant<AZStd::monostate, Value, Path> m_value;
Path m_domPath;
Type m_type;
AZStd::variant<AZStd::monostate, Value, Path> m_value;
};
AZ_DEFINE_ENUM_BITWISE_OPERATORS(PatchOperation::ExistenceCheckFlags);
class Patch;
//! The current state of a Patch application operation.
@@ -184,8 +189,18 @@ namespace AZ::Dom
Patch m_inversePatches;
};
//! Parameters for GenerateHierarchicalDeltaPatch.
struct DeltaPatchGenerationParameters
{
static constexpr size_t NoReplace = AZStd::numeric_limits<size_t>::max();
//! The threshold of changed values in a node or array which, if exceeded, will cause the generation to create an
//! entire "replace" oepration instead. If set to NoReplace, no replacement will occur.
size_t m_replaceThreshold = 3;
};
//! Generates a set of patches such that m_forwardPatches.Apply(beforeState) shall produce a document equivalent to afterState, and
//! a subsequent m_inversePatches.Apply(beforeState) shall produce the original document. This patch generation strategy does a
//! hierarchical comparison and is not guaranteed to create the minimal set of patches required to transform between the two states.
PatchInfo GenerateHierarchicalDeltaPatch(const Value& beforeState, const Value& afterState);
PatchInfo GenerateHierarchicalDeltaPatch(const Value& beforeState, const Value& afterState, const DeltaPatchGenerationParameters& params = {});
} // namespace AZ::Dom
Code/Framework/AzCore/AzCore/DOM/DomPath.cpp
+13 -5
View File
@@ -54,19 +54,19 @@ namespace AZ::Dom
bool PathEntry::operator==(size_t value) const
{
const size_t* internalValue = AZStd::get_if<size_t>(&m_value);
return internalValue == nullptr ? false : (*internalValue) == value;
return internalValue != nullptr && *internalValue == value;
}
bool PathEntry::operator==(const AZ::Name& key) const
{
const AZ::Name* internalValue = AZStd::get_if<AZ::Name>(&m_value);
return internalValue == nullptr ? false : (*internalValue) == key;
return internalValue != nullptr && *internalValue == key;
}
bool PathEntry::operator==(AZStd::string_view key) const
{
const AZ::Name* internalValue = AZStd::get_if<AZ::Name>(&m_value);
return internalValue == nullptr ? false : (*internalValue) == AZ::Name(key);
return internalValue != nullptr && *internalValue == AZ::Name(key);
}
bool PathEntry::operator!=(const PathEntry& other) const
@@ -323,13 +323,13 @@ namespace AZ::Dom
return size;
}
void Path::FormatString(char* stringBuffer, size_t bufferSize) const
size_t Path::FormatString(char* stringBuffer, size_t bufferSize) const
{
size_t bufferIndex = 0;
auto putChar = [&](char c)
{
if (bufferIndex == bufferSize)
if (bufferIndex >= bufferSize)
{
return;
}
@@ -360,6 +360,11 @@ namespace AZ::Dom
for (const PathEntry& entry : m_entries)
{
if (bufferIndex >= bufferSize)
{
return bufferIndex;
}
putChar(PathSeparator);
if (entry.IsEndOfArray())
{
@@ -375,7 +380,10 @@ namespace AZ::Dom
}
}
size_t bytesWritten = bufferIndex;
putChar('\0');
return bytesWritten;
}
AZStd::string Path::ToString() const
Code/Framework/AzCore/AzCore/DOM/DomPath.h
+10 -1
View File
@@ -128,10 +128,19 @@ namespace AZ::Dom
size_t GetStringLength() const;
//! Formats a JSON-pointer style path string into the target buffer.
//! This operation will fail if bufferSize < GetStringLength() + 1
void FormatString(char* stringBuffer, size_t bufferSize) const;
//! \return The number of bytes written, excepting the null terminator.
size_t FormatString(char* stringBuffer, size_t bufferSize) const;
//! Returns a JSON-pointer style path string for this path.
AZStd::string ToString() const;
void AppendToString(AZStd::string& output) const;
template <class T>
void AppendToString(T& output) const
{
const size_t startIndex = output.length();
output.resize_no_construct(startIndex + FormatString(output.data() + startIndex, output.capacity() - startIndex));
}
//! Reads a JSON-pointer style path from pathString and replaces this path's contents.
//! Paths are accepted in the following forms:
//! "/path/to/foo/0"
Code/Framework/AzCore/Tests/DOM/DomPathBenchmarks.cpp
+21
View File
@@ -96,4 +96,25 @@ namespace AZ::Dom::Benchmark
state.SetItemsProcessed(3 * state.iterations());
}
BENCHMARK_REGISTER_F(DomPathBenchmark, DomPathEntry_IsEndOfArray);
BENCHMARK_DEFINE_F(DomPathBenchmark, DomPathEntry_Comparison)(benchmark::State& state)
{
PathEntry name("name");
PathEntry index(0);
PathEntry endOfArray;
endOfArray.SetEndOfArray();
for (auto _ : state)
{
name == name;
name == index;
name == endOfArray;
index == index;
index == endOfArray;
endOfArray == endOfArray;
}
state.SetItemsProcessed(6 * state.iterations());
}
BENCHMARK_REGISTER_F(DomPathBenchmark, DomPathEntry_Comparison);
}
Code/Framework/AzCore/Tests/DOM/DomPathTests.cpp
+19
View File
@@ -174,4 +174,23 @@ namespace AZ::Dom::Tests
p.AppendToString(s);
EXPECT_EQ(s, "/foo/0/foo/0");
}
TEST_F(DomPathTests, MixedPath_AppendToFixedString)
{
Path p("/foo/0");
{
AZStd::fixed_string<7> s;
p.AppendToString(s);
EXPECT_EQ(s, "/foo/0");
}
{
AZStd::fixed_string<9> s;
p.AppendToString(s);
EXPECT_EQ(s, "/foo/0");
p.AppendToString(s);
EXPECT_EQ(s, "/foo/0/fo");
}
}
} // namespace AZ::Dom::Tests