Refactor the experimental RadixTree code #252

yungyuc · 2023-11-13T13:09:37Z

Fix the function and variable naming style to use snake_case from the wrong camelCase.
Use single-name accessor naming style when possible.
Add proper type aliases and static assertion.
Remove dead code.
Move RadixTreeNode::info() to testing code because the function is only used in testing.
Use namespace (alias mm = modmesh) in C++ testing code, which is considered API reference.

* Fix the function and variable naming style to use snake_case from the wrong camelCase. * Use single-name accessor naming style when possible. * Add proper type aliases and static assertion. * Remove dead code. * Move RadixTreeNode::info() to testing code because the function is only used in testing. * Use namespace (alias mm = modmesh) in C++ testing code, which is considered API reference.

yungyuc

@q40603 , please take a look at the refactor. Let me know what you think by adding global and inline comments.

yungyuc · 2023-11-13T13:10:33Z

cpp/modmesh/toggle/RadixTree.hpp

-    RadixTreeNode(std::string name, int key)
-        : m_name(std::move(name))
+
+    using child_list_type = std::list<std::unique_ptr<RadixTreeNode<T>>>;


Use snake_case for internal type names. Also add back the explicit <T> in RadixTreeNode<T>.

Thanks for helping adding back explicit T, which I fully forgot.
But will there be any negative scenarios for aliasing without passing template parameter in this case?

It reads strange. Semantically it's the same with or without the template argument, IIRC. But one needs to consult the standard to be sure which is right. It hurts productivity. When in doubt, choose the least surprising way to write it.

yungyuc · 2023-11-13T13:10:51Z

cpp/modmesh/toggle/RadixTree.hpp

-        : m_name(std::move(name))
+
+    using child_list_type = std::list<std::unique_ptr<RadixTreeNode<T>>>;
+    using key_type = int32_t;


Add key_type so that in the future we can widen the index key type.

yungyuc · 2023-11-13T13:11:06Z

cpp/modmesh/toggle/RadixTree.hpp

+    using child_list_type = std::list<std::unique_ptr<RadixTreeNode<T>>>;
+    using key_type = int32_t;
+
+    static_assert(std::is_integral_v<key_type> && std::is_signed_v<key_type>, "signed integral required");


The key type needs to be a signed integer.

yungyuc · 2023-11-13T13:11:28Z

cpp/modmesh/toggle/RadixTree.hpp

-    T & getData() { return m_data; }
-    const T & getData() const { return m_data; }
-    const ChildList & getChildren() const { return m_children; }
+    RadixTreeNode() = default;


Supplement all constructors.

yungyuc · 2023-11-13T13:12:01Z

cpp/modmesh/toggle/RadixTree.hpp

-            oss << getData();
-        }
-        return oss.str();
+        return m_prev;
    }


Move Info() to the test file because it is only used for testing.

yungyuc · 2023-11-13T13:12:38Z

cpp/modmesh/toggle/RadixTree.hpp

+    RadixTreeNode & operator=(RadixTreeNode &&) = default;
+    ~RadixTreeNode() = default;
+
+    key_type key() const { return m_key; }


Use single-name accessor style.

yungyuc · 2023-11-13T13:13:01Z

cpp/modmesh/toggle/RadixTree.hpp


-    // Add a child node
-    RadixTreeNode<T> * addChild(std::string childName, int childKey)
+    RadixTreeNode<T> * add_child(std::string const & name, key_type key)


Member functions use snake_case naming style, as other code in modmesh does.

yungyuc · 2023-11-13T13:13:23Z

gtests/CMakeLists.txt

@@ -22,7 +22,7 @@ add_executable(
    test_nopython_buffer.cpp
    test_nopython_modmesh.cpp
    test_nopython_inout.cpp
-    test_nonpython_radixtree.cpp
+    test_nopython_radixtree.cpp


Fix the file name.

yungyuc · 2023-11-13T13:13:40Z

gtests/test_nopython_radixtree.cpp

+#endif
+
+template <typename T>
+std::string get_info(modmesh::RadixTreeNode<T> & r)


Helper for printing the contents.

But if the tests should work like example, how can the user know what class members to be printed.
Also, when developing, should debug printing function also be added into unit tests?

Keep in mind that the RadixTree is still a prototype. It does not work for the profiling and stack printing yet. It does not have a Python wrapper yet. The usability cannot be defined yet. It is too early to evaluate how users will use the information. Because we do not have the information or experience for this feature, we do not add fragile code.

Adding fragile code in the tests is less intrusive, so we are doing it.

yungyuc · 2023-11-13T13:14:34Z

gtests/test_nopython_radixtree.cpp

+
+TEST(RadixTree, construction)
+{
+    namespace mm = modmesh;


The tests also work as examples for C++ API. We do not want users to use the library by using the namespace. Thus, use mm = modmesh alias here.

q40603 · 2023-11-14T12:53:29Z

cpp/modmesh/toggle/RadixTree.hpp

        return (it != m_children.end()) ? it->get() : nullptr;
    }

-    // Get prev node
-    RadixTreeNode<T> * getPrev() const
+    RadixTreeNode<T> * get_prev() const


Is single-name accessor style also applicable to this?

I haven't decided, so leave it as a getter. get_prev and get_child look like a group so that I keep them in the same style. And I am not yet sure about whether a pointer or a reference should be returned. It makes more sense to return a reference with the single-name style.

Let's keep it as is for a while.

yungyuc added the refactor Change code without changing tests label Nov 13, 2023

yungyuc commented Nov 13, 2023

View reviewed changes

q40603 reviewed Nov 14, 2023

View reviewed changes

Fix one-liner style

59a8593

yungyuc merged commit a9cee5b into solvcon:master Nov 15, 2023
11 checks passed

yungyuc deleted the refactor/radixtree branch November 15, 2023 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the experimental RadixTree code #252

Refactor the experimental RadixTree code #252

yungyuc commented Nov 13, 2023

yungyuc left a comment

yungyuc Nov 13, 2023

q40603 Nov 14, 2023

yungyuc Nov 14, 2023

yungyuc Nov 13, 2023

yungyuc Nov 13, 2023

yungyuc Nov 13, 2023

yungyuc Nov 13, 2023

yungyuc Nov 13, 2023

yungyuc Nov 13, 2023

yungyuc Nov 13, 2023

yungyuc Nov 13, 2023

q40603 Nov 14, 2023

yungyuc Nov 14, 2023

yungyuc Nov 13, 2023

q40603 Nov 14, 2023

yungyuc Nov 14, 2023

Refactor the experimental RadixTree code #252

Refactor the experimental RadixTree code #252

Conversation

yungyuc commented Nov 13, 2023

yungyuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment