From 8554c6632258f093101e1e6cbf7b06b7b3e8c86b Mon Sep 17 00:00:00 2001 From: Michael Demoret Date: Tue, 22 Oct 2024 00:32:42 -0400 Subject: [PATCH] Style cleanup --- .../mrc/node/operators/combine_latest.hpp | 2 +- .../mrc/node/operators/with_latest_from.hpp | 2 +- cpp/mrc/include/mrc/node/operators/zip.hpp | 2 +- cpp/mrc/include/mrc/segment/builder.hpp | 6 +- cpp/mrc/include/mrc/segment/object.hpp | 26 ++++----- cpp/mrc/include/mrc/utils/tuple_utils.hpp | 5 ++ .../node/operators/test_combine_latest.cpp | 1 - cpp/mrc/tests/node/operators/test_router.cpp | 57 ++++++++++++------- .../node/operators/test_with_latest_from.cpp | 1 - cpp/mrc/tests/node/operators/test_zip.cpp | 1 - cpp/mrc/tests/node/test_nodes.hpp | 50 ++++++++-------- python/mrc/core/node.cpp | 18 +++--- python/tests/test_edges.py | 6 +- 13 files changed, 101 insertions(+), 76 deletions(-) diff --git a/cpp/mrc/include/mrc/node/operators/combine_latest.hpp b/cpp/mrc/include/mrc/node/operators/combine_latest.hpp index 5c6788cdd..81f6ae28a 100644 --- a/cpp/mrc/include/mrc/node/operators/combine_latest.hpp +++ b/cpp/mrc/include/mrc/node/operators/combine_latest.hpp @@ -71,7 +71,7 @@ class CombineLatestBase, OutputT> m_upstream_holders(build_ingress(const_cast(this), std::index_sequence_for{})) {} - virtual ~CombineLatestBase() = default; + ~CombineLatestBase() override = default; template std::shared_ptr>> get_sink() const diff --git a/cpp/mrc/include/mrc/node/operators/with_latest_from.hpp b/cpp/mrc/include/mrc/node/operators/with_latest_from.hpp index 305371186..2d55abf7a 100644 --- a/cpp/mrc/include/mrc/node/operators/with_latest_from.hpp +++ b/cpp/mrc/include/mrc/node/operators/with_latest_from.hpp @@ -89,7 +89,7 @@ class WithLatestFromBase, OutputT> m_upstream_holders(build_ingress(const_cast(this), std::index_sequence_for{})) {} - virtual ~WithLatestFromBase() = default; + ~WithLatestFromBase() override = default; template std::shared_ptr>> get_sink() const diff --git a/cpp/mrc/include/mrc/node/operators/zip.hpp b/cpp/mrc/include/mrc/node/operators/zip.hpp index afc01350a..02f87eeb7 100644 --- a/cpp/mrc/include/mrc/node/operators/zip.hpp +++ b/cpp/mrc/include/mrc/node/operators/zip.hpp @@ -116,7 +116,7 @@ class ZipBase, OutputT> : public ZipTypelessBase, m_queue_counts.fill(0); } - virtual ~ZipBase() = default; + ~ZipBase() override = default; template std::shared_ptr>> get_sink() const diff --git a/cpp/mrc/include/mrc/segment/builder.hpp b/cpp/mrc/include/mrc/segment/builder.hpp index 2c474a2a1..e7d93720f 100644 --- a/cpp/mrc/include/mrc/segment/builder.hpp +++ b/cpp/mrc/include/mrc/segment/builder.hpp @@ -480,9 +480,9 @@ void IBuilder::make_edge(SourceObjectT source, SinkObjectT sink) sink_sp_type_t, // Fallback to Sink deduced type SinkNodeTypeT>; // Fallback to Sink explicit hint using deduced_sink_type_t = first_non_void_type_t; // Fallback to Source explicit hint + SinkNodeTypeT, // Explicit type hint + source_sp_type_t, // Fallback to Source deduced type + SourceNodeTypeT>; // Fallback to Source explicit hint VLOG(2) << "Deduced source type: " << mrc::type_name() << std::endl; VLOG(2) << "Deduced sink type: " << mrc::type_name() << std::endl; diff --git a/cpp/mrc/include/mrc/segment/object.hpp b/cpp/mrc/include/mrc/segment/object.hpp index 8ba06e7b6..aecb9d355 100644 --- a/cpp/mrc/include/mrc/segment/object.hpp +++ b/cpp/mrc/include/mrc/segment/object.hpp @@ -126,7 +126,7 @@ struct ObjectPropertiesState // Will be set by the builder class when the object is added to a segment bool m_is_initialized{false}; - std::string m_name{""}; + std::string m_name; // The owning builder. Once set, name cannot be changed IBuilder* m_owning_builder{nullptr}; @@ -241,9 +241,9 @@ edge::IWritableAcceptor& ObjectProperties::writable_acceptor_typed() if (writable_acceptor == nullptr) { - LOG(ERROR) << "Failed to cast " << type_name() << " to " << "IWritableAcceptor<" - << std::string(mrc::type_name()) << ">" << "IWritableAcceptor<" - << ::mrc::type_name(base.writable_acceptor_type().full_type()) << ">."; + LOG(ERROR) << "Failed to cast " << type_name() << " to " + << "IWritableAcceptor<" << std::string(mrc::type_name()) << ">" + << "IWritableAcceptor<" << ::mrc::type_name(base.writable_acceptor_type().full_type()) << ">."; throw exceptions::MrcRuntimeError("Failed to cast Sink to requested IWritableAcceptor"); } @@ -258,9 +258,9 @@ edge::IWritableProvider& ObjectProperties::writable_provider_typed() if (writable_provider == nullptr) { - LOG(ERROR) << "Failed to cast " << type_name() << " to " << "IWritableProvider<" - << std::string(mrc::type_name()) << ">" << "IWritableProvider<" - << ::mrc::type_name(base.writable_provider_type().full_type()) << ">."; + LOG(ERROR) << "Failed to cast " << type_name() << " to " + << "IWritableProvider<" << std::string(mrc::type_name()) << ">" + << "IWritableProvider<" << ::mrc::type_name(base.writable_provider_type().full_type()) << ">."; throw exceptions::MrcRuntimeError("Failed to cast Sink to requested IWritableProvider"); } @@ -275,9 +275,9 @@ edge::IReadableAcceptor& ObjectProperties::readable_acceptor_typed() if (readable_acceptor == nullptr) { - LOG(ERROR) << "Failed to cast " << type_name() << " to " << "IReadableAcceptor<" - << std::string(mrc::type_name()) << ">" << "IReadableAcceptor<" - << ::mrc::type_name(base.readable_acceptor_type().full_type()) << ">."; + LOG(ERROR) << "Failed to cast " << type_name() << " to " + << "IReadableAcceptor<" << std::string(mrc::type_name()) << ">" + << "IReadableAcceptor<" << ::mrc::type_name(base.readable_acceptor_type().full_type()) << ">."; throw exceptions::MrcRuntimeError("Failed to cast Sink to requested IReadableAcceptor"); } @@ -292,9 +292,9 @@ edge::IReadableProvider& ObjectProperties::readable_provider_typed() if (readable_provider == nullptr) { - LOG(ERROR) << "Failed to cast " << type_name() << " to " << "IReadableProvider<" - << std::string(mrc::type_name()) << ">" << "IReadableProvider<" - << ::mrc::type_name(base.readable_provider_type().full_type()) << ">."; + LOG(ERROR) << "Failed to cast " << type_name() << " to " + << "IReadableProvider<" << std::string(mrc::type_name()) << ">" + << "IReadableProvider<" << ::mrc::type_name(base.readable_provider_type().full_type()) << ">."; throw exceptions::MrcRuntimeError("Failed to cast Sink to requested IReadableProvider"); } diff --git a/cpp/mrc/include/mrc/utils/tuple_utils.hpp b/cpp/mrc/include/mrc/utils/tuple_utils.hpp index 14abaed91..d80f65da7 100644 --- a/cpp/mrc/include/mrc/utils/tuple_utils.hpp +++ b/cpp/mrc/include/mrc/utils/tuple_utils.hpp @@ -66,6 +66,9 @@ void tuple_for_each(TupleT&& tuple, FuncT&& f) std::make_index_sequence>::value>()); } +// NOLINTBEGIN(readability-identifier-naming) +// Disable naming conventions for std library-like functions + /** * @brief Creates a tuple of N elements of type T. For example, `repeat_tuple_type` would be `std::tuple` @@ -95,4 +98,6 @@ class repeat_tuple_type template using repeat_tuple_type_t = typename repeat_tuple_type::type; +// NOLINTEND(readability-identifier-naming) + } // namespace mrc::utils diff --git a/cpp/mrc/tests/node/operators/test_combine_latest.cpp b/cpp/mrc/tests/node/operators/test_combine_latest.cpp index 472064460..021d375ef 100644 --- a/cpp/mrc/tests/node/operators/test_combine_latest.cpp +++ b/cpp/mrc/tests/node/operators/test_combine_latest.cpp @@ -17,7 +17,6 @@ #include "../../test_mrc.hpp" // IWYU pragma: associated #include "../test_nodes.hpp" -#include "gtest/gtest.h" #include "mrc/node/operators/combine_latest.hpp" diff --git a/cpp/mrc/tests/node/operators/test_router.cpp b/cpp/mrc/tests/node/operators/test_router.cpp index cb5767bd2..8098fc905 100644 --- a/cpp/mrc/tests/node/operators/test_router.cpp +++ b/cpp/mrc/tests/node/operators/test_router.cpp @@ -17,7 +17,6 @@ #include "../../test_mrc.hpp" // IWYU pragma: associated #include "../test_nodes.hpp" -#include "gtest/gtest.h" #include "mrc/exceptions/runtime_error.hpp" // for MrcRuntimeError #include "mrc/node/operators/router.hpp" @@ -56,9 +55,9 @@ class DerivedRouterBase : public BaseT void run() { - constexpr bool has_do_run = requires(this_t& t) { t.do_run(); }; + constexpr bool HasDoRun = requires(this_t& t) { t.do_run(); }; - if constexpr (has_do_run) + if constexpr (HasDoRun) { this->do_run(); } @@ -82,9 +81,9 @@ class DerivedLambdaRouter : public BaseT void run() { - constexpr bool has_do_run = requires(this_t& t) { t.do_run(); }; + constexpr bool HasDoRun = requires(this_t& t) { t.do_run(); }; - if constexpr (has_do_run) + if constexpr (HasDoRun) { this->do_run(); } @@ -141,41 +140,57 @@ class TestRouterTypes : public testing::Test } }; -using RouterTypes = ::testing::Types>, - node::DerivedRouterBase>, - node::DerivedRouterBase>, - node::DerivedRouterBase>, - node::DerivedLambdaRouter>, - node::DerivedLambdaRouter>, - node::DerivedLambdaRouter>, - node::DerivedLambdaRouter>>; +using router_types_t = ::testing::Types>, + node::DerivedRouterBase>, + node::DerivedRouterBase>, + node::DerivedRouterBase>, + node::DerivedLambdaRouter>, + node::DerivedLambdaRouter>, + node::DerivedLambdaRouter>, + node::DerivedLambdaRouter>>; class RouterTypesNameGenerator { public: template - static std::string GetName(int) + static std::string GetName(int /*unused*/) // NOLINT(readability-identifier-naming) { if constexpr (std::is_same_v>>) + { return "StaticComponentBase"; + } if constexpr (std::is_same_v>>) + { return "StaticRunnableBase"; + } if constexpr (std::is_same_v>>) + { return "DynamicComponentBase"; + } if constexpr (std::is_same_v>>) + { return "DynamicRunnableBase"; + } if constexpr (std::is_same_v>>) + { return "LambdaStaticComponent"; + } if constexpr (std::is_same_v>>) + { return "LambdaStaticRunnable"; + } if constexpr (std::is_same_v>>) + { return "LambdaDynamicComponent"; + } if constexpr (std::is_same_v>>) + { return "LambdaDynamicRunnable"; + } } }; -TYPED_TEST_SUITE(TestRouterTypes, RouterTypes, RouterTypesNameGenerator); +TYPED_TEST_SUITE(TestRouterTypes, router_types_t, RouterTypesNameGenerator); TYPED_TEST(TestRouterTypes, SourceToRouterToSinks) { @@ -243,9 +258,9 @@ TYPED_TEST(TestRouterTypes, AutomaticTypeConversion) sink1->run(); sink2->run(); - EXPECT_EQ((std::vector{0.0f, 3.0f, 6.0f, 9.0f}), sink0->get_values()); - EXPECT_EQ((std::vector{1.0f, 4.0f, 7.0f}), sink1->get_values()); - EXPECT_EQ((std::vector{2.0f, 5.0f, 8.0f}), sink2->get_values()); + EXPECT_EQ((std::vector{0.0F, 3.0F, 6.0F, 9.0F}), sink0->get_values()); + EXPECT_EQ((std::vector{1.0F, 4.0F, 7.0F}), sink1->get_values()); + EXPECT_EQ((std::vector{2.0F, 5.0F, 8.0F}), sink2->get_values()); } TYPED_TEST(TestRouterTypes, SourceComponentToRouterToDifferentSinks) @@ -267,9 +282,9 @@ TYPED_TEST(TestRouterTypes, SourceComponentToRouterToDifferentSinks) sink0->run(); sink2->run(); - EXPECT_EQ((std::vector{0.0f, 3.0f, 6.0f, 9.0f}), sink0->get_values()); - EXPECT_EQ((std::vector{1.0f, 4.0f, 7.0f}), sink1->get_values()); - EXPECT_EQ((std::vector{2.0f, 5.0f, 8.0f}), sink2->get_values()); + EXPECT_EQ((std::vector{0.0F, 3.0F, 6.0F, 9.0F}), sink0->get_values()); + EXPECT_EQ((std::vector{1.0F, 4.0F, 7.0F}), sink1->get_values()); + EXPECT_EQ((std::vector{2.0F, 5.0F, 8.0F}), sink2->get_values()); } else { diff --git a/cpp/mrc/tests/node/operators/test_with_latest_from.cpp b/cpp/mrc/tests/node/operators/test_with_latest_from.cpp index 1d46197e8..61edf9866 100644 --- a/cpp/mrc/tests/node/operators/test_with_latest_from.cpp +++ b/cpp/mrc/tests/node/operators/test_with_latest_from.cpp @@ -17,7 +17,6 @@ #include "../../test_mrc.hpp" // IWYU pragma: associated #include "../test_nodes.hpp" -#include "gtest/gtest.h" #include "mrc/node/operators/with_latest_from.hpp" #include "mrc/node/operators/zip.hpp" diff --git a/cpp/mrc/tests/node/operators/test_zip.cpp b/cpp/mrc/tests/node/operators/test_zip.cpp index 4a8870e3a..c0219381f 100644 --- a/cpp/mrc/tests/node/operators/test_zip.cpp +++ b/cpp/mrc/tests/node/operators/test_zip.cpp @@ -17,7 +17,6 @@ #include "../../test_mrc.hpp" // IWYU pragma: associated #include "../test_nodes.hpp" -#include "gtest/gtest.h" #include "mrc/exceptions/runtime_error.hpp" // for MrcRuntimeError #include "mrc/node/operators/zip.hpp" diff --git a/cpp/mrc/tests/node/test_nodes.hpp b/cpp/mrc/tests/node/test_nodes.hpp index 1f87f81ed..65ff424f8 100644 --- a/cpp/mrc/tests/node/test_nodes.hpp +++ b/cpp/mrc/tests/node/test_nodes.hpp @@ -15,6 +15,8 @@ * limitations under the License. */ +#pragma once + #include "../test_mrc.hpp" // IWYU pragma: associated #include "mrc/channel/buffered_channel.hpp" // IWYU pragma: keep @@ -65,6 +67,31 @@ using namespace std::chrono_literals; namespace mrc::node { +template +typename std::enable_if<(N >= sizeof...(T))>::type print_tuple(std::ostream& /*unused*/, + const std::tuple& /*unused*/) +{} + +template +typename std::enable_if<(N < sizeof...(T))>::type print_tuple(std::ostream& os, const std::tuple& tup) +{ + if (N != 0) + { + os << ", "; + } + os << std::get(tup); + print_tuple(os, tup); +} + +// Utility function to print tuples +template +std::ostream& operator<<(std::ostream& os, const std::tuple& tup) +{ + os << "["; + print_tuple<0>(os, tup); + return os << "]"; +} + template class EdgeReadableLambda : public edge::IEdgeReadable { @@ -457,27 +484,4 @@ class TestDynamicRouter : public DynamicRouterComponentBase return keys[t % keys.size()]; } }; - -template -typename std::enable_if<(n >= sizeof...(T))>::type print_tuple(std::ostream&, const std::tuple&) -{} - -template -typename std::enable_if<(n < sizeof...(T))>::type print_tuple(std::ostream& os, const std::tuple& tup) -{ - if (n != 0) - os << ", "; - os << std::get(tup); - print_tuple(os, tup); -} - -// Utility function to print tuples -template -std::ostream& operator<<(std::ostream& os, const std::tuple& tup) -{ - os << "["; - print_tuple<0>(os, tup); - return os << "]"; -} - } // namespace mrc::node diff --git a/python/mrc/core/node.cpp b/python/mrc/core/node.cpp index f93b0e308..de4a5830e 100644 --- a/python/mrc/core/node.cpp +++ b/python/mrc/core/node.cpp @@ -114,15 +114,15 @@ PYBIND11_MODULE(node, py_mod) { return make_node.template operator()<1>(name); } - else if (count == 2) + if (count == 2) { return make_node.template operator()<2>(name); } - else if (count == 3) + if (count == 3) { return make_node.template operator()<3>(name); } - else if (count == 4) + if (count == 4) { return make_node.template operator()<4>(name); } @@ -176,15 +176,15 @@ PYBIND11_MODULE(node, py_mod) { return make_node.template operator()<1>(name); } - else if (count == 2) + if (count == 2) { return make_node.template operator()<2>(name); } - else if (count == 3) + if (count == 3) { return make_node.template operator()<3>(name); } - else if (count == 4) + if (count == 4) { return make_node.template operator()<4>(name); } @@ -237,15 +237,15 @@ PYBIND11_MODULE(node, py_mod) { return make_node.template operator()<1>(name); } - else if (count == 2) + if (count == 2) { return make_node.template operator()<2>(name); } - else if (count == 3) + if (count == 3) { return make_node.template operator()<3>(name); } - else if (count == 4) + if (count == 4) { return make_node.template operator()<4>(name); } diff --git a/python/tests/test_edges.py b/python/tests/test_edges.py index 2b5296b29..8c1bf2704 100644 --- a/python/tests/test_edges.py +++ b/python/tests/test_edges.py @@ -259,7 +259,11 @@ def add_router(seg: mrc.Builder, is_component: bool): if (key_fn is None): - key_fn = lambda _: "a" + + def inner_key_fn(x): + return "a" + + key_fn = inner_key_fn if (is_component): node = mrc.core.node.RouterComponent(seg, "RouterComponent", router_keys=router_keys, key_fn=key_fn)