pw_channel: Rework inheritance to avoid SiblingCast

Change pw_channel to a new inheritance pattern that places AnyChannel in
the middle of the hierarchy. The various Channel API classes are private
empty bases of AnyChannel. This ensures conversions between compatible
Channel variants are valid, since all channel implementations have all
channel variants as bases. Channel implementations extend a ChannelImpl
with properties specified in template parameters, and unsupported
functions are automatically disabled.

This change also refactors and simplifies the channel.h header.
Properties and code for checking them are moved to a properties.h
header. Also, the macros to generate ChannelImpl specializations are
greatly simplified.

Change-Id: Ib48bca41ec63ac107bf703b2102f03e9d6cd0347
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/247732
Pigweed-Auto-Submit: Wyatt Hepler <hepler@google.com>
Reviewed-by: Taylor Cramer <cramertj@google.com>
Commit-Queue: Wyatt Hepler <hepler@google.com>
Lint: Lint 🤖 <android-build-ayeaye@system.gserviceaccount.com>

pw_bluetooth/public/pw_bluetooth/controller2.h

@@ -28,7 +28,8 @@ namespace pw::bluetooth {
 /// Controller. Controller is a `pw::Channel` used to send and receive HCI
 /// packets. The first byte of each datagram is a UART packet indicator
 /// (`H4PacketType` Emboss enum).
-class Controller2 : public pw::channel::ReliableDatagramReaderWriter {
+class Controller2
+    : public pw::channel::Implement<pw::channel::ReliableDatagramReaderWriter> {
  public:
   /// Bitmask of features the controller supports.
   enum class FeaturesBits : uint32_t {

pw_channel/BUILD.bazel

@@ -21,7 +21,10 @@ licenses(["notice"])
 cc_library(
     name = "pw_channel",
     srcs = ["public/pw_channel/internal/channel_specializations.h"],
-    hdrs = ["public/pw_channel/channel.h"],
+    hdrs = [
+        "public/pw_channel/channel.h",
+        "public/pw_channel/properties.h",
+    ],
     includes = ["public"],
     deps = [
         "//pw_assert",

pw_channel/BUILD.gn

@@ -28,7 +28,10 @@ config("public_include_path") {
 
 pw_source_set("pw_channel") {
   public_configs = [ ":public_include_path" ]
-  public = [ "public/pw_channel/channel.h" ]
+  public = [
+    "public/pw_channel/channel.h",
+    "public/pw_channel/properties.h",
+  ]
   public_deps = [
     "$dir_pw_async2:dispatcher",
     "$dir_pw_async2:poll",

pw_channel/CMakeLists.txt

@@ -18,6 +18,7 @@ include($ENV{PW_ROOT}/pw_async2/backend.cmake)
 pw_add_library(pw_channel STATIC
   HEADERS
     public/pw_channel/channel.h
+    public/pw_channel/properties.h
   PUBLIC_DEPS
     pw_assert
     pw_async2.dispatcher

pw_channel/channel_test.cc

@@ -15,13 +15,13 @@
 
 #include <optional>
 
-#include "gtest/gtest.h"
 #include "pw_allocator/testing.h"
 #include "pw_assert/check.h"
 #include "pw_compilation_testing/negative_compilation.h"
 #include "pw_multibuf/allocator.h"
 #include "pw_multibuf/simple_allocator.h"
 #include "pw_preprocessor/compiler.h"
+#include "pw_unit_test/framework.h"
 
 namespace {
 
@@ -50,7 +50,7 @@ static_assert((kReliable < kReadable) && (kReadable < kWritable) &&
               (kWritable < kSeekable));
 
 class ReliableByteReaderWriterStub
-    : public pw::channel::ByteChannel<kReliable, kReadable, kWritable> {
+    : public pw::channel::ByteChannelImpl<kReliable, kReadable, kWritable> {
  private:
   // Read functions
 
@@ -83,7 +83,7 @@ class ReliableByteReaderWriterStub
   Poll<pw::Status> DoPendClose(Context&) override { return pw::OkStatus(); }
 };
 
-class ReadOnlyStub : public pw::channel::ByteReader {
+class ReadOnlyStub : public pw::channel::Implement<pw::channel::ByteReader> {
  public:
   constexpr ReadOnlyStub() = default;
 
@@ -96,7 +96,7 @@ class ReadOnlyStub : public pw::channel::ByteReader {
   Poll<pw::Status> DoPendClose(Context&) override { return pw::OkStatus(); }
 };
 
-class WriteOnlyStub : public pw::channel::ByteWriter {
+class WriteOnlyStub : public pw::channel::Implement<pw::channel::ByteWriter> {
  private:
   // Write functions
 
@@ -165,19 +165,30 @@ TEST(Channel, WriteOnlyChannelOnlyOpenForWrites) {
   EXPECT_TRUE(write_only.is_write_open());
 }
 
-#if PW_NC_TEST(InvalidOrdering)
+#if PW_NC_TEST(ChannelInvalidOrdering)
 PW_NC_EXPECT("Properties must be specified in the following order");
 bool Illegal(pw::channel::ByteChannel<kReadable, pw::channel::kReliable>& foo) {
   return foo.is_read_open();
 }
-#elif PW_NC_TEST(NoProperties)
+#elif PW_NC_TEST(ChannelImplInvalidOrdering)
+PW_NC_EXPECT("Properties must be specified in the following order");
+class BadChannel
+    : public pw::channel::ByteChannelImpl<kReadable, pw::channel::kReliable> {};
+#elif PW_NC_TEST(ChannelNoProperties)
 PW_NC_EXPECT("At least one of kReadable or kWritable must be provided");
 bool Illegal(pw::channel::ByteChannel<>& foo) { return foo.is_read_open(); }
-#elif PW_NC_TEST(NoReadOrWrite)
+#elif PW_NC_TEST(ChannelImplNoProperties)
+PW_NC_EXPECT("At least one of kReadable or kWritable must be provided");
+class NoChannel : public pw::channel::ByteChannelImpl<> {};
+#elif PW_NC_TEST(ChannelNoReadOrWrite)
 PW_NC_EXPECT("At least one of kReadable or kWritable must be provided");
 bool Illegal(pw::channel::ByteChannel<pw::channel::kReliable>& foo) {
   return foo.is_read_open();
 }
+#elif PW_NC_TEST(ChannelImplNoReadOrWrite)
+PW_NC_EXPECT("At least one of kReadable or kWritable must be provided");
+class BadChannel : public pw::channel::ByteChannelImpl<pw::channel::kReliable> {
+};
 #elif PW_NC_TEST(TooMany)
 PW_NC_EXPECT("Too many properties given");
 bool Illegal(
@@ -193,7 +204,8 @@ bool Illegal(pw::channel::ByteChannel<kReadable, kReadable>& foo) {
 }
 #endif  // PW_NC_TEST
 
-class TestByteReader : public ByteChannel<kReliable, kReadable> {
+class TestByteReader
+    : public pw::channel::ByteChannelImpl<kReliable, kReadable> {
  public:
   TestByteReader() {}
 
@@ -223,7 +235,7 @@ class TestByteReader : public ByteChannel<kReliable, kReadable> {
   MultiBuf data_;
 };
 
-class TestDatagramWriter : public DatagramWriter {
+class TestDatagramWriter : public pw::channel::Implement<DatagramWriter> {
  public:
   TestDatagramWriter(MultiBufAllocator& alloc) : alloc_fut_(alloc) {}
 
@@ -420,7 +432,7 @@ TEST(Channel, TestDatagramWriter) {
     DatagramWriter& channel_;
   };
 
-  SendWriteDataAndFlush test_task(write_channel, 24601);
+  SendWriteDataAndFlush test_task(write_channel.channel(), 24601);
   dispatcher.Post(test_task);
 
   EXPECT_EQ(dispatcher.RunUntilStalled(), Pending());
@@ -461,23 +473,42 @@ TEST(Channel, Conversions) {
   const TestByteReader byte_channel;
   const TestDatagramWriter datagram_channel(simple_allocator);
 
-  TakesAReadableByteChannel(byte_channel);
+  TakesAReadableByteChannel(byte_channel.channel());
+
   TakesAReadableByteChannel(byte_channel.as<kReadable>());
+  TakesAReadableByteChannel(byte_channel.channel().as<kReadable>());
+
   TakesAReadableByteChannel(byte_channel.as<pw::channel::ByteReader>());
+  TakesAReadableByteChannel(
+      byte_channel.channel().as<pw::channel::ByteReader>());
+
   TakesAReadableByteChannel(
       byte_channel.as<pw::channel::ByteChannel<kReliable, kReadable>>());
+  TakesAReadableByteChannel(
+      byte_channel.channel()
+          .as<pw::channel::ByteChannel<kReliable, kReadable>>());
+
   TakesAChannel(byte_channel);
+  TakesAChannel(byte_channel.as<pw::channel::AnyChannel>());
 
   TakesAWritableByteChannel(datagram_channel.IgnoreDatagramBoundaries());
+  TakesAWritableByteChannel(
+      datagram_channel.channel().IgnoreDatagramBoundaries());
 
   [[maybe_unused]] const pw::channel::AnyChannel& plain = byte_channel;
 
 #if PW_NC_TEST(CannotImplicitlyLoseWritability)
   PW_NC_EXPECT("no matching function for call");
-  TakesAWritableByteChannel(byte_channel);
+  TakesAWritableByteChannel(byte_channel.channel());
 #elif PW_NC_TEST(CannotExplicitlyLoseWritability)
   PW_NC_EXPECT("Cannot use a non-writable channel as a writable channel");
   TakesAWritableByteChannel(byte_channel.as<kWritable>());
+#elif PW_NC_TEST(CannotIgnoreDatagramBoundariesOnByteChannel)
+  PW_NC_EXPECT("only be called to use a datagram channel to a byte channel");
+  std::ignore = byte_channel.IgnoreDatagramBoundaries();
+#elif PW_NC_TEST(CannotIgnoreDatagramBoundariesOnByteChannelImpl)
+  PW_NC_EXPECT("only be called to use a datagram channel to a byte channel");
+  std::ignore = byte_channel.channel().IgnoreDatagramBoundaries();
 #endif  // PW_NC_TEST
 }
 
@@ -511,9 +542,32 @@ class Foo {
 TEST(Channel, SelectsCorrectOverloadWhenRelyingOnImplicitConversion) {
   TestByteReader byte_channel;
 
-  [[maybe_unused]] Foo selects_channel_ctor_not_copy_ctor(byte_channel);
+  [[maybe_unused]] Foo selects_channel_ctor_not_copy_ctor(
+      byte_channel.channel());
   EXPECT_EQ(&byte_channel.as<pw::channel::ByteChannel<kReadable>>(),
-            &TakesAReadableByteChannel(byte_channel));
+            &TakesAReadableByteChannel(byte_channel.channel()));
+}
+
+#if PW_NC_TEST(CannotCallUnsupportedWriteMethodsOnChannel)
+PW_NC_EXPECT("PendReadyToWrite may only be called on writable channels");
+[[maybe_unused]] void Bad(Context& cx, pw::channel::DatagramReader& c) {
+  std::ignore = c.PendReadyToWrite(cx);
 }
+#elif PW_NC_TEST(CannotCallUnsupportedWriteMethodsOnChannelImpl)
+PW_NC_EXPECT("PendReadyToWrite may only be called on writable channels");
+[[maybe_unused]] void Bad(Context& cx, pw::channel::ByteReaderImpl& c) {
+  std::ignore = c.PendReadyToWrite(cx);
+}
+#elif PW_NC_TEST(CannotCallUnsupportedReadMethodsOnChannel)
+PW_NC_EXPECT("PendRead may only be called on readable channels");
+[[maybe_unused]] void Bad(Context& cx, pw::channel::ByteWriter& c) {
+  std::ignore = c.PendRead(cx);
+}
+#elif PW_NC_TEST(CannotCallUnsupportedReadMethodsOnChannelImpl)
+PW_NC_EXPECT("PendRead may only be called on readable channels");
+[[maybe_unused]] void Bad(Context& cx, pw::channel::DatagramWriterImpl& c) {
+  std::ignore = c.PendRead(cx);
+}
+#endif  // PW_NC_TEST
 
 }  // namespace

pw_channel/epoll_channel_test.cc

@@ -142,7 +142,7 @@ TEST_F(EpollChannelTest, Read_ValidData_Succeeds) {
   ASSERT_TRUE(channel.is_read_open());
   ASSERT_TRUE(channel.is_write_open());
 
-  ReaderTask<ByteReader> read_task(channel, 1);
+  ReaderTask<ByteReader> read_task(channel.channel(), 1);
   dispatcher.Post(read_task);
 
   EXPECT_EQ(dispatcher.RunUntilStalled(), Pending());
@@ -184,7 +184,7 @@ TEST_F(EpollChannelTest, Read_Closed_ReturnsFailedPrecondition) {
   EXPECT_EQ(dispatcher.RunUntilStalled(), Ready());
   EXPECT_EQ(close_task.close_status, pw::OkStatus());
 
-  ReaderTask<ByteReader> read_task(channel, 1);
+  ReaderTask<ByteReader> read_task(channel.channel(), 1);
   dispatcher.Post(read_task);
 
   EXPECT_EQ(dispatcher.RunUntilStalled(), Ready());
@@ -257,7 +257,7 @@ TEST_F(EpollChannelTest, Write_ValidData_Succeeds) {
   ASSERT_TRUE(channel.is_write_open());
 
   constexpr auto kData = pw::bytes::Initialized<32>(0x3f);
-  WriterTask<ByteWriter> write_task(channel, 1, kData);
+  WriterTask<ByteWriter> write_task(channel.channel(), 1, kData);
   dispatcher.Post(write_task);
 
   dispatcher.RunToCompletion();
@@ -282,7 +282,7 @@ TEST_F(EpollChannelTest, Write_EmptyData_Succeeds) {
   ASSERT_TRUE(channel.is_read_open());
   ASSERT_TRUE(channel.is_write_open());
 
-  WriterTask<ByteWriter> write_task(channel, 1, {});
+  WriterTask<ByteWriter> write_task(channel.channel(), 1, {});
   dispatcher.Post(write_task);
 
   dispatcher.RunToCompletion();
@@ -307,7 +307,7 @@ TEST_F(EpollChannelTest, Write_Closed_ReturnsFailedPrecondition) {
   EXPECT_EQ(dispatcher.RunUntilStalled(), Ready());
   EXPECT_EQ(close_task.close_status, pw::OkStatus());
 
-  WriterTask<ByteWriter> write_task(channel, 1, {});
+  WriterTask<ByteWriter> write_task(channel.channel(), 1, {});
   dispatcher.Post(write_task);
 
   dispatcher.RunToCompletion();
@@ -339,7 +339,7 @@ TEST_F(EpollChannelTest, PendReadyToWrite_BlocksWhenUnavailable) {
   constexpr auto kData =
       pw::bytes::Initialized<decltype(alloc)::data_size_bytes()>('c');
   WriterTask<ByteWriter> write_task(
-      channel,
+      channel.channel(),
       100,  // Max writes set to some high number so the task fills the pipe.
       pw::ConstByteSpan(kData));
   dispatcher.Post(write_task);

pw_channel/loopback_channel_test.cc

@@ -69,7 +69,7 @@ class ReaderTask : public Task {
 TEST(LoopbackDatagramChannel, LoopsEmptyDatagrams) {
   SimpleAllocatorForTest alloc;
   LoopbackDatagramChannel channel(alloc);
-  ReaderTask<DatagramReader> read_task(channel);
+  ReaderTask<DatagramReader> read_task(channel.channel());
 
   Dispatcher dispatcher;
   dispatcher.Post(read_task);
@@ -89,7 +89,7 @@ TEST(LoopbackDatagramChannel, LoopsEmptyDatagrams) {
 TEST(LoopbackDatagramChannel, LoopsDatagrams) {
   SimpleAllocatorForTest alloc;
   LoopbackDatagramChannel channel(alloc);
-  ReaderTask<DatagramReader> read_task(channel);
+  ReaderTask<DatagramReader> read_task(channel.channel());
 
   Dispatcher dispatcher;
   dispatcher.Post(read_task);
@@ -109,7 +109,7 @@ TEST(LoopbackDatagramChannel, LoopsDatagrams) {
 TEST(LoopbackByteChannel, IgnoresEmptyWrites) {
   SimpleAllocatorForTest alloc;
   LoopbackByteChannel channel(alloc);
-  ReaderTask<ReliableByteReader> read_task(channel);
+  ReaderTask<ReliableByteReader> read_task(channel.channel());
 
   Dispatcher dispatcher;
   dispatcher.Post(read_task);
@@ -129,7 +129,7 @@ TEST(LoopbackByteChannel, IgnoresEmptyWrites) {
 TEST(LoopbackByteChannel, LoopsData) {
   SimpleAllocatorForTest alloc;
   LoopbackByteChannel channel(alloc);
-  ReaderTask<ReliableByteReader> read_task(channel);
+  ReaderTask<ReliableByteReader> read_task(channel.channel());
 
   Dispatcher dispatcher;
   dispatcher.Post(read_task);