From e644245128388fc3599d5528f430a69cc7f0e9b0 Mon Sep 17 00:00:00 2001 From: Jens Alfke Date: Sun, 4 Aug 2024 12:04:26 -0700 Subject: [PATCH] Added some [[nodiscard]], coro lifetime attributes - Added [[nodiscard]] to Coroutine, Error, Result, etc. - static_ASYNC, virtual_ASYNC macros no longer necessary - Added coro_return_type_ etc. but they aren't implemented in the version of Clang in Xcode 16 --- include/crouton/CoCondition.hh | 4 ++-- include/crouton/Coroutine.hh | 4 ++-- include/crouton/CroutonFwd.hh | 8 ++------ include/crouton/Error.hh | 2 +- include/crouton/EventLoop.hh | 2 +- include/crouton/Future.hh | 3 ++- include/crouton/Generator.hh | 2 +- include/crouton/PubSub.hh | 2 +- include/crouton/Result.hh | 2 +- include/crouton/Scheduler.hh | 6 +++--- include/crouton/io/AddrInfo.hh | 2 +- include/crouton/io/FileStream.hh | 2 +- include/crouton/io/ISocket.hh | 6 +++--- include/crouton/io/IStream.hh | 18 +++++++++--------- include/crouton/io/Stream.hh | 4 ++-- include/crouton/io/apple/NWConnection.hh | 8 ++++---- include/crouton/util/Base.hh | 24 ++++++++++++++++++++++++ src/io/blip/Connection.cc | 4 ++-- src/io/uv/Stream.cc | 2 +- src/support/Logging.cc | 2 +- tests/ESP32/main/test_esp32.cc | 4 +--- tests/demo_blipclient.cc | 4 ++-- tests/demo_client.cc | 4 ++-- tests/demo_server.cc | 6 +++--- tests/test_blip.cc | 2 +- tests/test_io.cc | 2 +- tests/tests.cc | 4 ++-- 27 files changed, 76 insertions(+), 57 deletions(-) diff --git a/include/crouton/CoCondition.hh b/include/crouton/CoCondition.hh index a4f9c7d..6d97bd4 100644 --- a/include/crouton/CoCondition.hh +++ b/include/crouton/CoCondition.hh @@ -94,7 +94,7 @@ namespace crouton { Blocker supports only one waiting coroutine. If you need more, use a CoCondition. */ template - class Blocker : public BlockerBase { + class [[nodiscard]] Blocker : public BlockerBase { public: T await_resume() noexcept { assert(_state == Ready); @@ -116,7 +116,7 @@ namespace crouton { template <> - class Blocker : public BlockerBase { + class [[nodiscard]] Blocker : public BlockerBase { public: void notify() {BlockerBase::notify();} }; diff --git a/include/crouton/Coroutine.hh b/include/crouton/Coroutine.hh index e98fb5a..c3e390c 100644 --- a/include/crouton/Coroutine.hh +++ b/include/crouton/Coroutine.hh @@ -30,7 +30,7 @@ namespace crouton { /** Base class for the public object returned by a coroutine function. Most of the implementation is usually in the associated CoroutineImpl subclass (IMPL). */ template - class Coroutine { + class [[nodiscard]] coro_return_type_ Coroutine { public: // movable, but not copyable. Coroutine(Coroutine&& c) noexcept :_handle(c._handle) {c._handle = {};} @@ -94,7 +94,7 @@ namespace crouton { - class CoroutineImplBase { + class coro_wrapper_ CoroutineImplBase { public: CoroutineImplBase() = default; ~CoroutineImplBase(); diff --git a/include/crouton/CroutonFwd.hh b/include/crouton/CroutonFwd.hh index 1a0e189..1ca3d6a 100644 --- a/include/crouton/CroutonFwd.hh +++ b/include/crouton/CroutonFwd.hh @@ -24,12 +24,8 @@ To be #include'd in header files instead of Crouton.hh, when possible. */ -/// Macros for declaring a function that returns a Future, e.g. `ASYNC close();` -/// It's surprisingly easy to forget to await the Future, especially `Future`, -/// hence the `[[nodiscard]]` annotation. -#define ASYNC [[nodiscard("Future must be AWAITed or returned")]] ::crouton::Future -#define staticASYNC [[nodiscard("Future must be AWAITed or returned")]] static ::crouton::Future -#define virtualASYNC [[nodiscard("Future must be AWAITed or returned")]] virtual ::crouton::Future +/// Marker macro for declaring a function that returns a Future, e.g. `ASYNC close();` +#define ASYNC ::crouton::Future namespace crouton { diff --git a/include/crouton/Error.hh b/include/crouton/Error.hh index d4efd22..431e5f2 100644 --- a/include/crouton/Error.hh +++ b/include/crouton/Error.hh @@ -65,7 +65,7 @@ namespace crouton { /** Error holds an error code as a type-erased enum value, of any type that matches `ErrorDomain`. There's also a default empty state. */ - class Error { + class [[nodiscard("check for error")]] Error { public: /// The default no-error value. Available as the constant `noerror`. constexpr Error() = default; diff --git a/include/crouton/EventLoop.hh b/include/crouton/EventLoop.hh index eb54c01..7afa839 100644 --- a/include/crouton/EventLoop.hh +++ b/include/crouton/EventLoop.hh @@ -82,7 +82,7 @@ namespace crouton { static void after(double delaySecs, std::function fn); /// Static method that returns a Future that completes after the given delay. - staticASYNC sleep(double delaySecs); + static ASYNC sleep(double delaySecs); private: friend class EventLoop; diff --git a/include/crouton/Future.hh b/include/crouton/Future.hh index 41336fb..e3fd514 100644 --- a/include/crouton/Future.hh +++ b/include/crouton/Future.hh @@ -53,7 +53,8 @@ namespace crouton { A regular function that gets a Future can call `then()` to register a callback. */ template - class Future : public Coroutine>, public ISelectable { + class [[nodiscard("Future must be AWAITed or returned")]] + Future : public Coroutine>, public ISelectable { public: using nonvoidT = std::conditional_t, std::byte, T>; diff --git a/include/crouton/Generator.hh b/include/crouton/Generator.hh index d7fdefa..0725f5a 100644 --- a/include/crouton/Generator.hh +++ b/include/crouton/Generator.hh @@ -42,7 +42,7 @@ namespace crouton { A Generator can also be iterated synchronously via its begin/end methods, or with a `for(:)` loop.*/ template - class Generator : public Coroutine>, public ISeries { + class [[nodiscard]] Generator : public Coroutine>, public ISeries { public: Generator(Generator&&) noexcept = default; Generator& operator=(Generator&&) = default; diff --git a/include/crouton/PubSub.hh b/include/crouton/PubSub.hh index 16428f3..cdfbf27 100644 --- a/include/crouton/PubSub.hh +++ b/include/crouton/PubSub.hh @@ -138,7 +138,7 @@ namespace crouton::ps { /// Abstract method that handles an item received from the Publisher. /// @warning You must override either this method or `run`. - virtualASYNC handle(T) {return CroutonError::Unimplemented;} + virtual ASYNC handle(T) {return CroutonError::Unimplemented;} /// Handles the final Error/noerror item from the publisher. /// Default implementation sets the `error` property; make sure to call through. diff --git a/include/crouton/Result.hh b/include/crouton/Result.hh index 2c3067a..9e8a97a 100644 --- a/include/crouton/Result.hh +++ b/include/crouton/Result.hh @@ -28,7 +28,7 @@ namespace crouton { (A `Result` has no explicit value, but still distinguishes between empty/nonempty.) It's commonly used as a return type, and in the implementation of `Future`. */ template - class Result { + class [[nodiscard("check Result for error")]] Result { public: using TT = std::conditional_t, std::monostate, T>; diff --git a/include/crouton/Scheduler.hh b/include/crouton/Scheduler.hh index 065fe99..61b5314 100644 --- a/include/crouton/Scheduler.hh +++ b/include/crouton/Scheduler.hh @@ -99,7 +99,7 @@ namespace crouton { //==== Awaitable: - class SchedAwaiter { + class SchedAwaiter { public: explicit SchedAwaiter(Scheduler* sched) :_sched(sched) { } bool await_ready() noexcept {return _sched->isCurrent();} @@ -138,12 +138,12 @@ namespace crouton { void resumed(coro_handle h); /// Returns the coroutine that should be resumed, or else `dflt`. - coro_handle nextOr(coro_handle dflt); + [[nodiscard]] coro_handle nextOr(coro_handle dflt); /// Adds a coroutine handle to the suspension set. /// To make it runnable again, call the returned Suspension's `wakeUp` method /// from any thread. - Suspension suspend(coro_handle h); + [[nodiscard]] Suspension suspend(coro_handle h); /// Tells the Scheduler a coroutine is about to be destroyed, so it can manage it /// correctly if it's in the suspended set. diff --git a/include/crouton/io/AddrInfo.hh b/include/crouton/io/AddrInfo.hh index ceb792d..6fcd8e6 100644 --- a/include/crouton/io/AddrInfo.hh +++ b/include/crouton/io/AddrInfo.hh @@ -38,7 +38,7 @@ namespace crouton::io { #endif /// Does a DNS lookup of the given hostname, returning an AddrInfo or an error. - staticASYNC lookup(string hostname, uint16_t port =0); + static ASYNC lookup(string hostname, uint16_t port =0); /// Returns the primary address, which may be either IPv4 or IPv6. RawAddress const& primaryAddress() const; diff --git a/include/crouton/io/FileStream.hh b/include/crouton/io/FileStream.hh index e52477c..0514b3b 100644 --- a/include/crouton/io/FileStream.hh +++ b/include/crouton/io/FileStream.hh @@ -60,7 +60,7 @@ namespace crouton::io { ASYNC readNoCopy(size_t maxLen = 65536) override; ASYNC peekNoCopy() override; - virtualASYNC readAll() override; + virtual ASYNC readAll() override; ASYNC write(ConstBytes) override; ASYNC write(const ConstBytes buffers[], size_t nBuffers) override; diff --git a/include/crouton/io/ISocket.hh b/include/crouton/io/ISocket.hh index 6fb2df5..fef091b 100644 --- a/include/crouton/io/ISocket.hh +++ b/include/crouton/io/ISocket.hh @@ -48,10 +48,10 @@ namespace crouton::io { virtual void keepAlive(unsigned intervalSecs) {_binding->keepAlive = intervalSecs;} /// Opens the socket to the bound address. Resolves once opened. - virtualASYNC open() =0; + virtual ASYNC open() =0; /// Equivalent to bind + open. - virtualASYNC connect(string const& address, uint16_t port); + virtual ASYNC connect(string const& address, uint16_t port); /// True if the socket is open/connected. virtual bool isOpen() const =0; @@ -59,7 +59,7 @@ namespace crouton::io { /// The socket's data stream. virtual std::shared_ptr stream() =0; - virtualASYNC close() =0; + virtual ASYNC close() =0; virtual ~ISocket() = default; diff --git a/include/crouton/io/IStream.hh b/include/crouton/io/IStream.hh index 2d00c71..630e1e0 100644 --- a/include/crouton/io/IStream.hh +++ b/include/crouton/io/IStream.hh @@ -38,13 +38,13 @@ namespace crouton::io { virtual bool isOpen() const =0; /// Opens the stream, if it wasn't already open; resolves when it's open. - virtualASYNC open() =0; + virtual ASYNC open() =0; /// Closes the stream; resolves when it's closed. - virtualASYNC close() =0; + virtual ASYNC close() =0; /// Closes the write side, but not the read side. (Like a socket's `shutdown`.) - virtualASYNC closeWrite() =0; + virtual ASYNC closeWrite() =0; //---- Reading: @@ -53,7 +53,7 @@ namespace crouton::io { /// @warning The returned buffer belongs to the stream, and is only valid until the next /// read or close call. /// @note This is an abstract method that subclasses must implement. - virtualASYNC readNoCopy(size_t maxLen = 65536) =0; + virtual ASYNC readNoCopy(size_t maxLen = 65536) =0; /// Returns the next available unread bytes, always at least 1 byte except at EOF. /// Unlike `readNoCopy`, the returned bytes are _not consumed_ -- they will @@ -62,11 +62,11 @@ namespace crouton::io { /// @warning The returned buffer belongs to the stream, and is only valid until the next /// read or close call. /// @note This is an abstract method that subclasses must implement. - virtualASYNC peekNoCopy() =0; + virtual ASYNC peekNoCopy() =0; /// Reads `len` bytes, copying into memory starting at `dst` (which must remain valid.) /// Will always read the full number of bytes unless it hits EOF. - virtualASYNC read(MutableBytes buf); + virtual ASYNC read(MutableBytes buf); /// Reads `len` bytes, returning them as a string. /// Will always read the full number of bytes unless it hits EOF. @@ -81,7 +81,7 @@ namespace crouton::io { ASYNC readUntil(string end, size_t maxLen = SIZE_MAX); /// Reads until EOF. - virtualASYNC readAll(); + virtual ASYNC readAll(); /// Returns a `Generator` that produces chunks of data read from the stream. /// This is a wrapper around `readNoCopy()` that makes `IStream` satisfy the concept @@ -94,7 +94,7 @@ namespace crouton::io { /// Writes all the bytes. /// @warning The memory must remain valid until this call completes. /// @note This is the abstract write method that subclasses must implement. - virtualASYNC write(ConstBytes) =0; + virtual ASYNC write(ConstBytes) =0; /// Writes data, fully. The string is copied, so the caller doesn't need to keep it. ASYNC write(string); @@ -104,7 +104,7 @@ namespace crouton::io { /// @note The default implementation makes `nBuffers` calls to `write(ConstBytes)`. /// A subclass that natively supports multi-buffer write ("writev") may override /// this method as an optimization. - virtualASYNC write(const ConstBytes buffers[], size_t nBuffers); + virtual ASYNC write(const ConstBytes buffers[], size_t nBuffers); ASYNC write(std::initializer_list buffers); }; diff --git a/include/crouton/io/Stream.hh b/include/crouton/io/Stream.hh index 3813b89..a2e3020 100644 --- a/include/crouton/io/Stream.hh +++ b/include/crouton/io/Stream.hh @@ -36,8 +36,8 @@ namespace crouton::io { /// Closes the write stream, leaving the read stream open until the peer closes it. ASYNC closeWrite() override; - /// Closes the stream entirely. (Called by the destructor.) - virtual Future close() override; + /// Closes the stream entirely. + virtual ASYNC close() override; //---- READING diff --git a/include/crouton/io/apple/NWConnection.hh b/include/crouton/io/apple/NWConnection.hh index 08822bf..cc45488 100644 --- a/include/crouton/io/apple/NWConnection.hh +++ b/include/crouton/io/apple/NWConnection.hh @@ -46,7 +46,7 @@ namespace crouton::io::apple { void useTLS(bool tls) {_useTLS = tls;} /// Opens the socket to the bound address. Resolves once opened. - virtualASYNC open() override; + virtual ASYNC open() override; bool isOpen() const override {return _isOpen;} @@ -60,12 +60,12 @@ namespace crouton::io::apple { ~NWConnection(); private: - virtualASYNC readNoCopy(size_t maxLen = 65536) override; - virtualASYNC peekNoCopy() override; + virtual ASYNC readNoCopy(size_t maxLen = 65536) override; + virtual ASYNC peekNoCopy() override; ASYNC write(ConstBytes b) override; using IStream::write; - virtualASYNC _readNoCopy(size_t maxLen, bool peek); + virtual ASYNC _readNoCopy(size_t maxLen, bool peek); ASYNC _writeOrShutdown(ConstBytes, bool shutdown); void _close(); void clearReadBuf(); diff --git a/include/crouton/util/Base.hh b/include/crouton/util/Base.hh index 50dbf47..c1e1d1a 100644 --- a/include/crouton/util/Base.hh +++ b/include/crouton/util/Base.hh @@ -70,6 +70,30 @@ #endif +// These will be helpful for detecting bugs, but AppleClang doesn't support them yet as of Xcode 16. +// +#if __has_attribute(coro_return_type) +# define coro_return_type_ [[clang::coro_return_type]] +#else +# define coro_return_type_ +#endif +#if __has_attribute(coro_wrapper) +# define coro_wrapper_ [[clang::coro_wrapper]] +#else +# define coro_wrapper_ +#endif +#if __has_attribute(coro_lifetimebound) +# define coro_lifetimebound_ [[clang::coro_lifetimebound]] +#else +# define coro_lifetimebound_ +#endif +#if __has_attribute(coro_disable_lifetimebound) +# define coro_disable_lifetimebound_ [[clang::coro_disable_lifetimebound]] +#else +# define coro_disable_lifetimebound_ +#endif + + // Synonyms for coroutine primitives. Optional, but they're more visible in the code. #define AWAIT co_await #define YIELD co_yield diff --git a/src/io/blip/Connection.cc b/src/io/blip/Connection.cc index b724667..eb65d70 100644 --- a/src/io/blip/Connection.cc +++ b/src/io/blip/Connection.cc @@ -84,11 +84,11 @@ namespace crouton::io::blip { else _io.closeSend(); auto& j = _outputTask->join(); - AWAIT j; + (void) AWAIT j; LBLIP->debug("Connection now sending WebSocket CLOSE..."); AWAIT _socket->send(ws::Message{code, message}); auto& j2 = _inputTask->join(); - AWAIT j2; + (void) AWAIT j2; AWAIT _socket->close(); RETURN noerror; } diff --git a/src/io/uv/Stream.cc b/src/io/uv/Stream.cc index b739149..c15a14e 100644 --- a/src/io/uv/Stream.cc +++ b/src/io/uv/Stream.cc @@ -30,7 +30,7 @@ namespace crouton::io { Stream::Stream() = default; Stream::~Stream() { - close(); + _close(); } diff --git a/src/support/Logging.cc b/src/support/Logging.cc index c511f2d..2f1075a 100644 --- a/src/support/Logging.cc +++ b/src/support/Logging.cc @@ -120,7 +120,7 @@ namespace crouton { assert_failed_hook = [](const char* message) { Log->critical(message); }; - Log->info("---------- Welcome to Crouton ----------"); + //Log->info("---------- Welcome to Crouton ----------"); }); } diff --git a/tests/ESP32/main/test_esp32.cc b/tests/ESP32/main/test_esp32.cc index 8c73a58..aacfd18 100644 --- a/tests/ESP32/main/test_esp32.cc +++ b/tests/ESP32/main/test_esp32.cc @@ -73,7 +73,7 @@ static void testCodec() { } -staticASYNC testBLIP() { +static ASYNC testBLIP() { // Send HTTP request: auto ws = make_unique("ws://work.local:4985/travel-sample/_blipsync"); ws->setHeader("Sec-WebSocket-Protocol", "BLIP_3+CBMobile_2"); @@ -235,5 +235,3 @@ static void initialize() { esp_log_level_set("example_common", ESP_LOG_WARN); ESP_ERROR_CHECK(example_connect()); } - - diff --git a/tests/demo_blipclient.cc b/tests/demo_blipclient.cc index 36ccbed..f355782 100644 --- a/tests/demo_blipclient.cc +++ b/tests/demo_blipclient.cc @@ -28,13 +28,13 @@ using namespace crouton::mini; /* NOTE to newbies: this file uses some simple/optional macros that are used everywhere in Crouton to highlight suspend points and use of asynchronous code: - staticASYNC --> [[nodiscard]] static Future + static ASYNC --> [[nodiscard]] static Future AWAIT --> co_await RETURN --> co_return */ -staticASYNC run() { +static ASYNC run() { // Read flags: auto args = MainArgs(); string protocol; diff --git a/tests/demo_client.cc b/tests/demo_client.cc index 6a2469a..9019303 100644 --- a/tests/demo_client.cc +++ b/tests/demo_client.cc @@ -26,13 +26,13 @@ using namespace crouton::io; /* NOTE to newbies: this file uses some simple/optional macros that are used everywhere in Crouton to highlight suspend points and use of asynchronous code: - staticASYNC --> [[nodiscard]] static Future + static ASYNC --> [[nodiscard]] static Future AWAIT --> co_await RETURN --> co_return */ -staticASYNC run() { +static ASYNC run() { // Read flags: auto args = MainArgs(); bool includeHeaders = false; diff --git a/tests/demo_server.cc b/tests/demo_server.cc index ccdfbd2..3f6c64a 100644 --- a/tests/demo_server.cc +++ b/tests/demo_server.cc @@ -31,7 +31,7 @@ using namespace crouton::io; /* NOTE to newbies: this file uses some simple/optional macros that are used everywhere in Crouton to highlight suspend points and use of asynchronous code: - staticASYNC --> [[nodiscard]] static Future + static ASYNC --> [[nodiscard]] static Future AWAIT --> co_await RETURN --> co_return */ @@ -39,14 +39,14 @@ using namespace crouton::io; static constexpr uint16_t kPort = 34567; -staticASYNC serveRoot(http::Handler::Request const& req, http::Handler::Response& res) { +static ASYNC serveRoot(http::Handler::Request const& req, http::Handler::Response& res) { res.writeHeader("Content-Type", "text/plain"); AWAIT res.writeToBody("Hi!\r\n"); RETURN noerror; } -staticASYNC serveWebSocket(http::Handler::Request const& req, http::Handler::Response& res) { +static ASYNC serveWebSocket(http::Handler::Request const& req, http::Handler::Response& res) { ws::ServerWebSocket socket; if (! AWAIT socket.connect(req, res)) RETURN noerror; diff --git a/tests/test_blip.cc b/tests/test_blip.cc index c420d1f..1f22019 100644 --- a/tests/test_blip.cc +++ b/tests/test_blip.cc @@ -82,7 +82,7 @@ TEST_CASE("BLIP Receive Message", "[blip]") { } -staticASYNC testSendReceive(std::initializer_list properties, +static ASYNC testSendReceive(std::initializer_list properties, string body, bool compressed) { diff --git a/tests/test_io.cc b/tests/test_io.cc index b799abe..75c20eb 100644 --- a/tests/test_io.cc +++ b/tests/test_io.cc @@ -239,7 +239,7 @@ TEST_CASE("readdir", "[uv]") { #ifdef __APPLE__ -staticASYNC readNWSocket(const char* hostname, bool tls) { +static ASYNC readNWSocket(const char* hostname, bool tls) { cerr << "Connecting...\n"; auto socket = apple::NWConnection::create(); socket->bind(hostname, (tls ? 443 : 80)); diff --git a/tests/tests.cc b/tests/tests.cc index f4564f5..d34cbba 100644 --- a/tests/tests.cc +++ b/tests/tests.cc @@ -270,7 +270,7 @@ TEST_CASE("Producer Consumer") { #if 0 -staticASYNC waitFor(chrono::milliseconds ms) { +static ASYNC waitFor(chrono::milliseconds ms) { FutureProvider f; Timer::after(ms.count() / 1000.0, [f] { cerr << "\tTimer fired\n"; @@ -303,7 +303,7 @@ TEST_CASE("Waiter coroutine") { } -staticASYNC futuristicSquare(int n) { +static ASYNC futuristicSquare(int n) { AWAIT waitFor(500ms); RETURN n * n; }