refactor(clp_s): delete clp_s::FileReader #711
Conversation
WalkthroughThis pull request removes the Changes
Sequence Diagram(s)sequenceDiagram
participant CMD as CommandLineArguments
participant FR as clp::FileReader
participant Log as Logger
CMD->>FR: Instantiate clp::FileReader(file_path)
alt Successful file read
FR-->>CMD: Return file content
else Read failure (exception)
FR-->>CMD: Throw OperationFailed exception
CMD->>Log: Log error via SPDLOG_ERROR
end
Possibly related issues
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 (
|
![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 (4)
components/core/src/clp_s/ZstdDecompressor.cpp (1)
291-293: Style: Usefalse ==for consistency with coding guidelines.While the error handling logic is correct, let's align the condition with the coding guidelines.
- clp::ErrorCode::ErrorCode_Success != rc && clp::ErrorCode::ErrorCode_EndOfFile != rc) + false == (clp::ErrorCode::ErrorCode_Success == rc || clp::ErrorCode::ErrorCode_EndOfFile == rc))components/core/src/clp_s/CommandLineArguments.cpp (2)
36-47: Consider trimming whitespace for each line read.When reading lines from the file, you could trim leading and trailing whitespace to guard against accidental spaces or hidden characters. This is a minor refinement that may help ensure cleaner path entries.
error_code = reader.try_read_to_delimiter('\n', false, false, line); if (clp::ErrorCode::ErrorCode_Success != error_code) { break; } -if (false == line.empty()) { - path_destination.push_back(line); -} +{ + std::string trimmed = clp::trim(line); // hypothetical trim utility + if (false == trimmed.empty()) { + path_destination.push_back(trimmed); + } }
54-59: Refine the logged error message for clarity.The catch block logs a message indicating a failure to “open” the file, but the exception may also occur mid-read. Consider revising the message to reflect that any file read operation could fail.
SPDLOG_ERROR( - "Failed to open file for reading - {} - {}", + "File operation failed - {} - {}", input_path_list_file_path, e.what() );components/core/src/clp_s/ArchiveWriter.cpp (1)
191-191: Consider marking the function asnoexcept(false).The static analysis tool indicates that this function might throw exceptions but isn't marked appropriately.
Apply this change to the function declaration:
-void ArchiveWriter::write_archive_files( +void ArchiveWriter::write_archive_files( FileWriter& archive_writer, - std::vector<ArchiveFileInfo> const& files + std::vector<ArchiveFileInfo> const& files) noexcept(false)🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 191-191: Exception thrown in function declared not to throw exceptions.
(throwInNoexceptFunction)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
components/core/CMakeLists.txt(0 hunks)components/core/src/clp_s/ArchiveWriter.cpp(1 hunks)components/core/src/clp_s/CMakeLists.txt(0 hunks)components/core/src/clp_s/CommandLineArguments.cpp(2 hunks)components/core/src/clp_s/Decompressor.hpp(2 hunks)components/core/src/clp_s/FileReader.cpp(0 hunks)components/core/src/clp_s/FileReader.hpp(0 hunks)components/core/src/clp_s/JsonParser.hpp(0 hunks)components/core/src/clp_s/SchemaReader.hpp(0 hunks)components/core/src/clp_s/TimestampDictionaryReader.hpp(0 hunks)components/core/src/clp_s/ZstdDecompressor.cpp(3 hunks)components/core/src/clp_s/ZstdDecompressor.hpp(3 hunks)components/core/src/clp_s/indexer/CMakeLists.txt(0 hunks)
💤 Files with no reviewable changes (8)
- components/core/src/clp_s/SchemaReader.hpp
- components/core/src/clp_s/TimestampDictionaryReader.hpp
- components/core/src/clp_s/JsonParser.hpp
- components/core/src/clp_s/indexer/CMakeLists.txt
- components/core/src/clp_s/FileReader.hpp
- components/core/src/clp_s/CMakeLists.txt
- components/core/src/clp_s/FileReader.cpp
- components/core/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (5)
components/core/src/clp_s/ArchiveWriter.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/Decompressor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/ZstdDecompressor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/ZstdDecompressor.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/CommandLineArguments.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🪛 Cppcheck (2.10-2)
components/core/src/clp_s/ArchiveWriter.cpp
[error] 191-191: Exception thrown in function declared not to throw exceptions.
(throwInNoexceptFunction)
🔇 Additional comments (11)
components/core/src/clp_s/ZstdDecompressor.hpp (3)
12-12: LGTM! Include directive added for the new namespace.The include directive for
clp::FileReaderis correctly added, aligning with the PR's objective to removeclp_s::FileReader.
45-45: LGTM! Method signature updated to use clp::FileReader.The method signature has been correctly updated to use
clp::FileReader, as mentioned in the PR objectives.
134-134: LGTM! Member variable type updated to use clp::FileReader.The member variable type has been correctly updated to use
clp::FileReader, maintaining consistency with the method signature change.components/core/src/clp_s/ZstdDecompressor.cpp (3)
69-78: LGTM! Error handling improvements.The changes improve clarity by using fully qualified error codes and ensure type safety through explicit casting.
167-167: LGTM! Signature update aligns with PR objectives.The change from
FileReadertoclp::FileReaderis consistent with the PR's goal of removingclp_s::FileReader.
Line range hint
303-307: LGTM! Consistent error handling and style.The error handling for ClpReader follows the same pattern as FileReader and adheres to the coding guidelines.
components/core/src/clp_s/CommandLineArguments.cpp (2)
12-12: Header inclusion looks correct.Replacing the local
FileReaderheader withFileReader.hppfrom theclpnamespace aligns with the objective of removing the old file reading mechanism inclp_s.
50-53: Confirm error-handling for unexpected file read errors.The code returns
falsefor any error code that is notErrorCode_EndOfFile. Ensure that this behaviour covers partial readings or other scenarios where incomplete results might be acceptable.components/core/src/clp_s/ArchiveWriter.cpp (1)
182-191: LGTM! The changes align with the PR objectives.The code correctly transitions from
clp_s::FileReadertoclp::FileReaderand properly handles error codes with the new namespace. The removal of explicitclose()suggests proper RAII usage.Let's verify that all
FileReaderusages have been updated consistently:✅ Verification successful
FileReader usage is consistently updated.
All instances ofclp_s::FileReaderhave been removed, and the code now uniformly usesclp::FileReader. The RAII-based error handling appears in line with the intended design change.
- No remaining references to
clp_s::FileReaderwere found.- All
FileReaderimplementations are correctly namespaced.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all FileReader usages have been updated to use clp::FileReader # Test: Search for any remaining references to clp_s::FileReader rg -l "clp_s::FileReader" # Test: Verify all FileReader usages are properly namespaced ast-grep --pattern 'FileReader'Length of output: 858
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 191-191: Exception thrown in function declared not to throw exceptions.
(throwInNoexceptFunction)
components/core/src/clp_s/Decompressor.hpp (2)
8-8: LGTM! Include directive updated correctly.The include directive has been properly updated to use the FileReader from the clp namespace, which aligns with the PR objective of removing clp_s::FileReader.
52-52: LGTM! Method signature updated correctly.The parameter type has been properly updated to use clp::FileReader. Since this is a pure virtual method, we should verify that all derived classes have been updated accordingly.
Let's verify that all derived classes have been updated to use clp::FileReader:
✅ Verification successful
Approval Notice: Decompressor Interface and Its Derived Classes Updated Correctly
The change to use clp::FileReader for the open method’s parameter is correctly applied in clp_s/Decompressor.hpp. Inspection of derived classes (e.g. in ZstdDecompressor) confirms that the override using clp::FileReader is present. There are additional overloads (e.g. one taking clp::ReaderInterface) but they do not conflict with the intended update.
- clp_s/Decompressor.hpp now declares:
virtual void open(clp::FileReader& file_reader, size_t file_read_buffer_capacity) = 0;- Derived classes, such as ZstdDecompressor, correctly override the open method with a matching signature.
LGTM!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all derived classes of Decompressor and verify their open method signatures # Find all derived classes of Decompressor ast-grep --pattern 'class $_ : $$$public $$$Decompressor$$$' # Search for any remaining references to FileReader without the clp:: namespace rg -g '*.{cpp,hpp}' --no-heading 'FileReader[^:]' -A 2 -B 2Length of output: 59571
This PR follows the discussion in #664. I removed
clp_s/FileReader.hppandclp_s/FileReader.cppfrom the codebase and, consequently, any reference toclp_s/FileReader.hpp.For the reasons I specified here I have not removed the method
clp_s::ZstdDecompressor::open(FileReader&...), but I just updated it to take a reference ofclip::FileReader.Summary by CodeRabbit