Modify deprecation macro - #2429
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2429 +/- ##
==========================================
+ Coverage 73.29% 73.30% +0.01%
==========================================
Files 645 645
Lines 30801 30839 +38
Branches 3381 3384 +3
==========================================
+ Hits 22575 22608 +33
- Misses 6315 6318 +3
- Partials 1911 1913 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates CAF’s IO-network deprecation plumbing and related declarations to improve compatibility across compilers (notably MSVC) while also applying minor formatting and typo fixes.
Changes:
- Adjust
CAF_IO_NETWORK_DEPRECATEDto use MSVC-specific deprecation syntax and reflow its comment. - Reformat several deprecated class declarations and split
CAF_DEPRECATEDannotations onto separate lines forinspecttemplates. - Minor maintenance: include reordering in some headers, fix a typo in a test comment, and extend typos config.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libcaf_io/caf/io/network/stream.hpp | Reformat deprecated class declaration to a single line. |
| libcaf_io/caf/io/network/stream_manager.hpp | Reorder includes; reformat deprecated class declaration. |
| libcaf_io/caf/io/network/stream_impl.hpp | Reorder includes (moves caf_deprecated.hpp below stream include). |
| libcaf_io/caf/io/network/scribe_impl.hpp | Reformat deprecated class declaration to a single line. |
| libcaf_io/caf/io/network/operation.hpp | Split CAF_DEPRECATED attribute from function signature for inspect. |
| libcaf_io/caf/io/network/native_socket.hpp | Reformat invalid_native_socket constant to a single line. |
| libcaf_io/caf/io/network/multiplexer.hpp | Reformat deprecated class declarations (including nested runnable). |
| libcaf_io/caf/io/network/manager.hpp | Reformat deprecated class declaration to a single line. |
| libcaf_io/caf/io/network/ip_endpoint.hpp | Split CAF_DEPRECATED attribute from function signature for inspect. |
| libcaf_io/caf/io/network/interfaces.hpp | Reorder includes (moves caf_deprecated.hpp below other includes). |
| libcaf_io/caf/io/network/doorman_impl.hpp | Reformat deprecated class declaration to a single line. |
| libcaf_io/caf/io/network/datagram_manager.hpp | Reorder includes (moves caf_deprecated.hpp below other includes). |
| libcaf_io/caf/io/network/datagram_handler_impl.hpp | Reorder includes (moves caf_deprecated.hpp below other includes). |
| libcaf_io/caf/io/network/acceptor.hpp | Reformat deprecated class declaration to a single line. |
| libcaf_io/caf/io/network/acceptor_manager.hpp | Reorder includes (moves caf_deprecated.hpp below other includes). |
| libcaf_io/caf/io/network/acceptor_impl.hpp | Reorder includes (moves caf_deprecated.hpp below other includes). |
| libcaf_io/caf/detail/io_network_deprecated.hpp | Add MSVC-specific deprecation attribute branch for CAF_IO_NETWORK_DEPRECATED. |
| libcaf_core/caf/metaprogramming.test.cpp | Fix spelling in a comment (“incompatible”). |
| .typos-config.toml | Add cpy to typos dictionary mappings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Neverlord
left a comment
Neverlord
left a comment
There was a problem hiding this comment.
Please rebase/squash and file to main.
a7c6552 to
7e6e278
Compare
Neverlord
left a comment
Neverlord
left a comment
There was a problem hiding this comment.
We have dozens of repeated CAF_DEPRECATED("use caf.net instead") now. Since we are already introducing custom macros, why not have:
CAF_IO_NETWORK_DEPRECATEDas alias forCAF_DEPRECATED("use caf.net instead")CAF_IO_NETWORK_DEPRECATED_CLASSfor the current implementation
Further, please find a better way than sprinkling #define CAF_SUPPRESS_DEPRECATION_WARNINGS across the code base. We already pass libcaf_${name}_EXPORTS to our targets to make the export macros work. Why not pass a flag at the target level for our unit tests plus libcaf_io and libcaf_openssl?
Suppressing deprecation warnings on all of our unit tests seems to be a good idea anyway.
fbfaaf3 to
6712c97
Compare
| # define CAF_PUSH_DEPRECATED_WARNING \ | ||
| __pragma(warning(push)) | ||
| __pragma(warning(push)) \ | ||
| __pragma(warning(disable: 4996)) |
There was a problem hiding this comment.
This is not related to deprecation, but it disables MSVC deprecation warning. Previously, it was missing.
3ec8e43 to
3c44be9
Compare
| # The legacy I/O module is deprecated as a whole, so its examples cannot | ||
| # avoid naming deprecated APIs. | ||
| caf_silence_deprecation_warnings(${name}) |
There was a problem hiding this comment.
Not true, the I/O module is not deprecated. We are deprecating things inside caf::io::network, not the entire module.
| # The legacy I/O module is deprecated as a whole, so its examples cannot | |
| # avoid naming deprecated APIs. | |
| caf_silence_deprecation_warnings(${name}) |
The examples have to pass without any warnings. The namespace caf::io::network only contains utilities that are mostly intended for internal use. This set of utilities is now covered (better) by things in caf::net, that's the intend of the deprecation. There's probably very few (if any) users depending on caf::io::network directly. Existing code just using the regular caf/io/... headers must not suddenly start raising deprecation warnings.
After a quick glance, it looks like we need to un-deprecate these three classes:
- caf::io::network::address_listing
- caf::io::network::protocol
- caf::io::network::receive_buffer
We pass them to CAF_ADD_TYPE_ID in caf/io/fwd.hpp, so we can't really avoid users pulling that in when using the I/O module. The examples run into deprecations warnings via caf/io/middleman.hpp pulling in caf/io/network/multiplexer.hpp.
See if you can hide the multiplexer (i.e., not include its header from the middleman). The supervisor could just go into its own header instead of being a nested type. The tricky part is probably spawn_client_impl and spawn_server_impl because they access backend(). Maybe we can add a new interface with the bare minimum needed by the middleman and then have the multiplexer inherit from that?
If there's no reasonable way to avoid pulling in the multiplexer header then we'll need to un-deprecate as much as necessary to get the public API headers building again without warnings.
There was a problem hiding this comment.
I have made some changes in an attempt to deprecate the classes properly. It should not throw any deprecation warning if "caf/io/all.hpp" is called. But, warning will be thrown for io::network classes (Which are deprecated currently).
07a91dc to
ec026af
Compare
ec026af to
2201035
Compare
Closes #2258.