refactor(core): Modernize function signatures in the streaming decompressor interface and its implementations to align with the latest C++ coding guidelines. #702
Conversation
WalkthroughThe pull request introduces modifications to several header and implementation files within the Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Bill-hbrhbr
changed the title
Bill-hbrhbr
changed the title
Bill-hbrhbr
changed the title
Bill-hbrhbr
changed the title
Bill-hbrhbr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/core/src/clp/streaming_compression/zstd/Decompressor.hpp (2)
40-48: LGTM! Well-structured special member functions.The special member functions follow the Rule of Five correctly:
- Destructor is properly marked
override- Copy operations are explicitly deleted
- Move operations are defaulted and correctly marked
noexceptConsider grouping the special member functions together by moving the destructor declaration after the move assignment operator for better readability:
- ~Decompressor() override; - // Delete copy constructor and assignment operator Decompressor(Decompressor const&) = delete; auto operator=(Decompressor const&) -> Decompressor& = delete; // Default move constructor and assignment operator Decompressor(Decompressor&&) noexcept = default; auto operator=(Decompressor&&) noexcept -> Decompressor& = default; + ~Decompressor() override;
82-84: Consider omittingvoidreturn type.While the signatures are correct, modern C++ allows omitting the
-> voidreturn type specification.- auto open(char const* compressed_data_buf, size_t compressed_data_buf_size) -> void override; - auto open(ReaderInterface& reader, size_t read_buffer_capacity) -> void override; - auto close() -> void override; + void open(char const* compressed_data_buf, size_t compressed_data_buf_size) override; + void open(ReaderInterface& reader, size_t read_buffer_capacity) override; + void close() override;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/core/src/clp/WriterInterface.hpp(1 hunks)components/core/src/clp/streaming_compression/Compressor.hpp(1 hunks)components/core/src/clp/streaming_compression/Decompressor.hpp(2 hunks)components/core/src/clp/streaming_compression/passthrough/Decompressor.cpp(7 hunks)components/core/src/clp/streaming_compression/passthrough/Decompressor.hpp(4 hunks)components/core/src/clp/streaming_compression/zstd/Decompressor.cpp(9 hunks)components/core/src/clp/streaming_compression/zstd/Decompressor.hpp(5 hunks)
✅ Files skipped from review due to trivial changes (2)
- components/core/src/clp/streaming_compression/passthrough/Decompressor.cpp
- components/core/src/clp/streaming_compression/zstd/Decompressor.cpp
🧰 Additional context used
📓 Path-based instructions (5)
components/core/src/clp/WriterInterface.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/streaming_compression/Compressor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/streaming_compression/Decompressor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/streaming_compression/zstd/Decompressor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/streaming_compression/passthrough/Decompressor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (13)
components/core/src/clp/WriterInterface.hpp (1)
24-25: LGTM! Virtual destructor addition enhances class hierarchy safety.The addition of a virtual destructor is essential for proper cleanup when derived classes are deleted through base class pointers.
components/core/src/clp/streaming_compression/Compressor.hpp (1)
35-35: LGTM! Explicit override improves clarity.The explicit override keyword on the destructor clearly indicates the inheritance relationship with WriterInterface.
components/core/src/clp/streaming_compression/Decompressor.hpp (4)
21-24: LGTM! Enhanced method signature with [[nodiscard]] and trailing return type.The addition of [[nodiscard]] ensures the error message isn't accidentally ignored, and the trailing return type improves readability.
30-30: LGTM! Explicit override improves clarity.The explicit override keyword on the destructor clearly indicates the inheritance relationship.
32-38: LGTM! Well-defined special member functions.Explicitly declaring copy/move operations improves class design clarity:
- Copy operations properly deleted
- Move operations defaulted with noexcept
46-65: LGTM! Modernized method signatures.Methods updated with:
- Trailing return types for improved readability
- [[nodiscard]] attribute where appropriate
- Consistent formatting
components/core/src/clp/streaming_compression/passthrough/Decompressor.hpp (4)
22-25: LGTM! Enhanced method signature with [[nodiscard]] and trailing return type.Consistent with parent class changes, improving error handling safety.
36-36: LGTM! Explicit override improves clarity.Consistent with parent class destructor declaration.
38-44: LGTM! Well-defined special member functions.Special member functions follow modern C++ best practices and maintain consistency with parent class.
Line range hint
57-92: LGTM! Modernized method signatures.All interface method implementations consistently updated with:
- Trailing return types
- [[nodiscard]] attribute where appropriate
- Clear parameter documentation
components/core/src/clp/streaming_compression/zstd/Decompressor.hpp (3)
27-29: LGTM! Good use of modern C++ features.The updated signature properly uses
[[nodiscard]]to prevent accidental ignoring of error messages and adopts the trailing return type syntax.
63-64: LGTM! Proper use of[[nodiscard]]for error handling methods.The reader interface methods correctly use
[[nodiscard]]to ensure error codes are not accidentally ignored.Also applies to: 72-72, 79-79
94-98: LGTM! Consistent use of modern C++ features.The method signatures properly use:
[[nodiscard]]for methods returning error codes- Trailing return type syntax
overridespecifier where applicableAlso applies to: 109-109
Bill-hbrhbr
changed the title
|
Also I've noticed that there are still many unresolved clang-tidy warnings in the current source files. We shouldn't lose track of these issues; are you planning to fix them in the coming PRs soon? |
Yes this is the first PR of a series that is to come |
Bill-hbrhbr
changed the title
Can we create a GitHub issue to keep track of this PR series? |
Description
Use the following clang-tidy checks to fix all function signature issues:
Note that this is an overly thorough checklist regarding function signatures, but it's still a subset of the complete config.
Validation performed
Summary by CodeRabbit
Refactor
[[nodiscard]]attributes to improve code clarityCode Quality
The changes primarily focus on modernizing the codebase's syntax and improving type safety without altering core functionality.