Address review feedback:

- Use `unordred_map` (speed is about the same but providing a hash is slightly nicer than operator<)
- Do better arg forwarding where applicable
- Swap EXPECT_ test ordering

Signed-off-by: Nicholas Van Sickle <nvsickle@amazon.com>

Code/Framework/AzCore/AzCore/DOM/DomPath.cpp

@@ -89,26 +89,6 @@ namespace AZ::Dom
         return !operator==(key);
     }
 
-    bool PathEntry::operator<(const PathEntry& rhs) const
-    {
-        return AZStd::visit(
-            [&](auto&& lhsValue)
-            {
-                using CurrentType = AZStd::decay_t<decltype(lhsValue)>;
-                if constexpr (AZStd::is_same_v<CurrentType, size_t>)
-                {
-                    const size_t* rhsValue = AZStd::get_if<size_t>(&rhs.m_value);
-                    return rhsValue == nullptr ? true : lhsValue < *rhsValue;
-                }
-                else if constexpr (AZStd::is_same_v<CurrentType, AZ::Name>)
-                {
-                    const AZ::Name* rhsValue = AZStd::get_if<Name>(&rhs.m_value);
-                    return rhsValue == nullptr ? false : lhsValue.GetStringView() < rhsValue->GetStringView();
-                }
-            },
-            m_value);
-    }
-
     void PathEntry::SetEndOfArray()
     {
         m_value = EndOfArrayIndex;
@@ -137,6 +117,36 @@ namespace AZ::Dom
         return AZStd::get<size_t>(m_value);
     }
 
+    size_t PathEntry::GetHash() const
+    {
+        return AZStd::visit(
+            [&](auto&& value) -> size_t
+            {
+                using CurrentType = AZStd::decay_t<decltype(value)>;
+                if constexpr (AZStd::is_same_v<CurrentType, size_t>)
+                {
+                    AZStd::hash<size_t> hasher;
+                    return hasher(value);
+                }
+                else if constexpr (AZStd::is_same_v<CurrentType, AZ::Name>)
+                {
+                    return value.GetHash();
+                }
+            },
+            m_value);
+    }
+} // namespace AZ::Dom
+
+namespace AZStd
+{
+    size_t AZStd::hash<AZ::Dom::PathEntry>::operator()(const AZ::Dom::PathEntry& entry) const
+    {
+        return entry.GetHash();
+    }
+} // namespace AZStd
+
+namespace AZ::Dom
+{
     const AZ::Name& PathEntry::GetKey() const
     {
         AZ_Assert(IsKey(), "Key called on PathEntry that is not a key");

Code/Framework/AzCore/AzCore/DOM/DomPath.h

@@ -47,10 +47,6 @@ namespace AZ::Dom
         bool operator!=(const AZ::Name& key) const;
         bool operator!=(AZStd::string_view key) const;
 
-        //! Comparison operator used for storage in map structures
-        //! Compares hash order and not lexigraphic order, so not suitable for UI purposes
-        bool operator<(const PathEntry& rhs) const;
-
         void SetEndOfArray();
 
         bool IsEndOfArray() const;
@@ -59,11 +55,24 @@ namespace AZ::Dom
 
         size_t GetIndex() const;
         const AZ::Name& GetKey() const;
+        size_t GetHash() const;
 
     private:
         AZStd::variant<size_t, AZ::Name> m_value;
     };
+} // namespace AZ::Dom
+
+namespace AZStd
+{
+    template<>
+    struct hash<AZ::Dom::PathEntry>
+    {
+        size_t operator()(const AZ::Dom::PathEntry& entry) const;
+    };
+} // namespace AZStd
 
+namespace AZ::Dom
+{
     //! Represents a path, represented as a series of PathEntry values, to a position in a Value.
     class Path final
     {
@@ -139,7 +148,7 @@ namespace AZ::Dom
         AZStd::string ToString() const;
 
         void AppendToString(AZStd::string& output) const;
-        template <class T>
+        template<class T>
         void AppendToString(T& output) const
         {
             const size_t startIndex = output.length();

Code/Framework/AzCore/AzCore/DOM/DomPrefixTree.h

@@ -15,7 +15,7 @@
 
 namespace AZ::Dom
 {
-    //! Specifies how a patch matches against a DomPrefixTree
+    //! Specifies how a path matches against a DomPrefixTree
     enum class PrefixTreeMatch
     {
         //! Only an exact path will match.
@@ -44,17 +44,16 @@ namespace AZ::Dom
         explicit DomPrefixTree(AZStd::initializer_list<AZStd::pair<Path, T>> init);
 
         //! Visits a path and calls a visitor for each matching path and value.
-        void VisitPath(const Path& path, PrefixTreeMatch match, AZStd::function<void(const Path&, const T&)> visitor) const;
+        void VisitPath(const Path& path, PrefixTreeMatch match, const AZStd::function<void(const Path&, const T&)>& visitor) const;
         //! Visits a path and returns the most specific matching value, or null if none was found.
         T* ValueAtPath(const Path& path, PrefixTreeMatch match);
         //! \see ValueAtPath
         const T* ValueAtPath(const Path& path, PrefixTreeMatch match) const;
         //! Visits a path and returns the most specific matching value or some default value.
-        //! \note This returns a copy of a value. If T is expensive to copy, consider using ValueAtPath instead.
+        T ValueAtPathOrDefault(const Path& path, T&& defaultValue, PrefixTreeMatch match) const;
-        T ValueAtPathOrDefault(const Path& path, const T& defaultValue, PrefixTreeMatch match) const;
 
         //! Sets the value stored at path.
-        void SetValue(const Path& path, T value);
+        void SetValue(const Path& path, T&& value);
         //! Removes the value stored at path. If removeChildren is true, also removes any values stored at subpaths.
         void EraseValue(const Path& path, bool removedChildren = false);
         //! Removes all entries from this tree.
@@ -63,7 +62,7 @@ namespace AZ::Dom
     private:
         struct Node
         {
-            AZStd::map<PathEntry, Node> m_values;
+            AZStd::unordered_map<PathEntry, Node> m_values;
             AZStd::optional<T> m_data;
         };
 

Code/Framework/AzCore/AzCore/DOM/DomPrefixTree.inl

@@ -52,7 +52,7 @@ namespace AZ::Dom
     }
 
     template<class T>
-    void DomPrefixTree<T>::VisitPath(const Path& path, PrefixTreeMatch match, AZStd::function<void(const Path&, const T&)> visitor) const
+    void DomPrefixTree<T>::VisitPath(const Path& path, PrefixTreeMatch match, const AZStd::function<void(const Path&, const T&)>& visitor) const
     {
         const Node* rootNode = GetNodeForPath(path);
         if (rootNode == nullptr)
@@ -166,14 +166,14 @@ namespace AZ::Dom
     }
 
     template<class T>
-    T DomPrefixTree<T>::ValueAtPathOrDefault(const Path& path, const T& defaultValue, PrefixTreeMatch match) const
+    T DomPrefixTree<T>::ValueAtPathOrDefault(const Path& path, T&& defaultValue, PrefixTreeMatch match) const
     {
         const T* value = ValueAtPath(path, match);
-        return value == nullptr ? defaultValue : *value;
+        return value == nullptr ? AZStd::forward<T>(defaultValue) : *value;
     }
 
     template<class T>
-    void DomPrefixTree<T>::SetValue(const Path& path, T value)
+    void DomPrefixTree<T>::SetValue(const Path& path, T&& value)
     {
         Node* node = &m_rootNode;
         for (const PathEntry& entry : path)
@@ -181,7 +181,7 @@ namespace AZ::Dom
             // Get or create an entry in this node
             node = &node->m_values[entry];
         }
-        node->m_data = value;
+        node->m_data = AZStd::forward<T>(value);
     }
 
     template<class T>

Code/Framework/AzCore/Tests/DOM/DomPrefixTreeTests.cpp

@@ -17,7 +17,7 @@ namespace AZ::Dom::Tests
     {
         DomPrefixTree<AZStd::string> tree;
         tree.SetValue(Path(), "root");
-        EXPECT_EQ(*tree.ValueAtPath(Path(), PrefixTreeMatch::ExactPath), "root");
+        EXPECT_EQ("root", *tree.ValueAtPath(Path(), PrefixTreeMatch::ExactPath));
     }
 
     TEST_F(DomPrefixTreeTests, GetExactPath)
@@ -29,14 +29,14 @@ namespace AZ::Dom::Tests
         tree.SetValue(Path("/foo/foo"), 1);
         tree.SetValue(Path("/foo/bar"), 2);
 
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::ExactPath), 0);
+        EXPECT_EQ(0, *tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::ExactPath));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/1"), PrefixTreeMatch::ExactPath), 42);
+        EXPECT_EQ(42, *tree.ValueAtPath(Path("/foo/1"), PrefixTreeMatch::ExactPath));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/foo"), PrefixTreeMatch::ExactPath), 1);
+        EXPECT_EQ(1, *tree.ValueAtPath(Path("/foo/foo"), PrefixTreeMatch::ExactPath));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/bar"), PrefixTreeMatch::ExactPath), 2);
+        EXPECT_EQ(2, *tree.ValueAtPath(Path("/foo/bar"), PrefixTreeMatch::ExactPath));
 
-        EXPECT_EQ(tree.ValueAtPath(Path(), PrefixTreeMatch::ExactPath), nullptr);
+        EXPECT_EQ(nullptr, tree.ValueAtPath(Path(), PrefixTreeMatch::ExactPath));
-        EXPECT_EQ(tree.ValueAtPath(Path("/foo"), PrefixTreeMatch::ExactPath), nullptr);
+        EXPECT_EQ(nullptr, tree.ValueAtPath(Path("/foo"), PrefixTreeMatch::ExactPath));
-        EXPECT_EQ(tree.ValueAtPath(Path("/foo/0/subpath"), PrefixTreeMatch::ExactPath), nullptr);
+        EXPECT_EQ(nullptr, tree.ValueAtPath(Path("/foo/0/subpath"), PrefixTreeMatch::ExactPath));
     }
 
     TEST_F(DomPrefixTreeTests, GetSubpath)
@@ -46,12 +46,12 @@ namespace AZ::Dom::Tests
         tree.SetValue(Path("/foo/0"), 0);
         tree.SetValue(Path("/foo/1"), 42);
 
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/0/bar"), PrefixTreeMatch::SubpathsOnly), 0);
+        EXPECT_EQ(0, *tree.ValueAtPath(Path("/foo/0/bar"), PrefixTreeMatch::SubpathsOnly));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/0/bar/baz"), PrefixTreeMatch::SubpathsOnly), 0);
+        EXPECT_EQ(0, *tree.ValueAtPath(Path("/foo/0/bar/baz"), PrefixTreeMatch::SubpathsOnly));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/1/0"), PrefixTreeMatch::SubpathsOnly), 42);
+        EXPECT_EQ(42, *tree.ValueAtPath(Path("/foo/1/0"), PrefixTreeMatch::SubpathsOnly));
 
-        EXPECT_EQ(tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::SubpathsOnly), nullptr);
+        EXPECT_EQ(nullptr, tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::SubpathsOnly));
-        EXPECT_EQ(tree.ValueAtPath(Path("/foo/1"), PrefixTreeMatch::SubpathsOnly), nullptr);
+        EXPECT_EQ(nullptr, tree.ValueAtPath(Path("/foo/1"), PrefixTreeMatch::SubpathsOnly));
     }
 
     TEST_F(DomPrefixTreeTests, GetPathOrSubpath)
@@ -61,15 +61,15 @@ namespace AZ::Dom::Tests
         tree.SetValue(Path("/foo/0"), 0);
         tree.SetValue(Path("/foo/1"), 42);
 
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::PathAndSubpaths), 0);
+        EXPECT_EQ(0, *tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::PathAndSubpaths));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/0/bar"), PrefixTreeMatch::PathAndSubpaths), 0);
+        EXPECT_EQ(0, *tree.ValueAtPath(Path("/foo/0/bar"), PrefixTreeMatch::PathAndSubpaths));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/0/bar/baz"), PrefixTreeMatch::PathAndSubpaths), 0);
+        EXPECT_EQ(0, *tree.ValueAtPath(Path("/foo/0/bar/baz"), PrefixTreeMatch::PathAndSubpaths));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/1"), PrefixTreeMatch::PathAndSubpaths), 42);
+        EXPECT_EQ(42, *tree.ValueAtPath(Path("/foo/1"), PrefixTreeMatch::PathAndSubpaths));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/1/0"), PrefixTreeMatch::PathAndSubpaths), 42);
+        EXPECT_EQ(42, *tree.ValueAtPath(Path("/foo/1/0"), PrefixTreeMatch::PathAndSubpaths));
 
-        EXPECT_EQ(tree.ValueAtPath(Path(), PrefixTreeMatch::PathAndSubpaths), nullptr);
+        EXPECT_EQ(nullptr, tree.ValueAtPath(Path(), PrefixTreeMatch::PathAndSubpaths));
-        EXPECT_EQ(tree.ValueAtPath(Path("/foo"), PrefixTreeMatch::PathAndSubpaths), nullptr);
+        EXPECT_EQ(nullptr, tree.ValueAtPath(Path("/foo"), PrefixTreeMatch::PathAndSubpaths));
-        EXPECT_EQ(tree.ValueAtPath(Path("/path/0"), PrefixTreeMatch::PathAndSubpaths), nullptr);
+        EXPECT_EQ(nullptr, tree.ValueAtPath(Path("/path/0"), PrefixTreeMatch::PathAndSubpaths));
     }
 
     TEST_F(DomPrefixTreeTests, RemovePath)
@@ -82,8 +82,8 @@ namespace AZ::Dom::Tests
 
         tree.EraseValue(Path("/foo"));
 
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo"), PrefixTreeMatch::PathAndSubpaths), 20);
+        EXPECT_EQ(20, *tree.ValueAtPath(Path("/foo"), PrefixTreeMatch::PathAndSubpaths));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::PathAndSubpaths), 80);
+        EXPECT_EQ(80, *tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::PathAndSubpaths));
     }
 
     TEST_F(DomPrefixTreeTests, RemovePathAndChildren)
@@ -96,8 +96,8 @@ namespace AZ::Dom::Tests
 
         tree.EraseValue(Path("/foo"), true);
 
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo"), PrefixTreeMatch::PathAndSubpaths), 20);
+        EXPECT_EQ(20, *tree.ValueAtPath(Path("/foo"), PrefixTreeMatch::PathAndSubpaths));
-        EXPECT_EQ(*tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::PathAndSubpaths), 20);
+        EXPECT_EQ(20, *tree.ValueAtPath(Path("/foo/0"), PrefixTreeMatch::PathAndSubpaths));
     }
 
     TEST_F(DomPrefixTreeTests, ClearTree)
@@ -109,7 +109,7 @@ namespace AZ::Dom::Tests
 
         tree.Clear();
 
-        EXPECT_EQ(tree.ValueAtPathOrDefault(Path("/foo"), -10, PrefixTreeMatch::PathAndSubpaths), -10);
+        EXPECT_EQ(-10, tree.ValueAtPathOrDefault(Path("/foo"), -10, PrefixTreeMatch::PathAndSubpaths));
     }
 
     TEST_F(DomPrefixTreeTests, Visit)
@@ -141,27 +141,27 @@ namespace AZ::Dom::Tests
         tree.SetValue(Path("/bar/baz"), 2);
 
         tree.VisitPath(Path("/bar"), PrefixTreeMatch::ExactPath, visitorFn);
-        EXPECT_EQ(results.size(), 0);
+        EXPECT_EQ(0, results.size());
         results.clear();
 
         tree.VisitPath(Path("/foo/0"), PrefixTreeMatch::ExactPath, visitorFn);
-        EXPECT_EQ(results.size(), 1);
+        EXPECT_EQ(1, results.size());
         EXPECT_TRUE(validateResult(Path("/foo/0"), 0));
         results.clear();
 
         tree.VisitPath(Path("/foo/1"), PrefixTreeMatch::ExactPath, visitorFn);
-        EXPECT_EQ(results.size(), 1);
+        EXPECT_EQ(1, results.size());
         EXPECT_TRUE(validateResult(Path("/foo/1"), 42));
         results.clear();
 
         tree.VisitPath(Path("/foo"), PrefixTreeMatch::SubpathsOnly, visitorFn);
-        EXPECT_EQ(results.size(), 2);
+        EXPECT_EQ(2, results.size());
         EXPECT_TRUE(validateResult(Path("/foo/0"), 0));
         EXPECT_TRUE(validateResult(Path("/foo/1"), 42));
         results.clear();
 
         tree.VisitPath(Path("/foo"), PrefixTreeMatch::PathAndSubpaths, visitorFn);
-        EXPECT_EQ(results.size(), 3);
+        EXPECT_EQ(3, results.size());
         EXPECT_TRUE(validateResult(Path("/foo"), 99));
         EXPECT_TRUE(validateResult(Path("/foo/0"), 0));
         EXPECT_TRUE(validateResult(Path("/foo/1"), 42));