refactor(lint): Enable linting for clp_s/Schema.hpp and clp_s/ColumnWriter.hpp; Remove unused ID field from BaseColumnWriter.#2042
Open
davidlion wants to merge 9 commits intoy-scope:mainfrom
Open
Conversation
…e unused id field from BaseColumnWriter.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
WalkthroughRemoved per-column id storage and id-based constructors from column writers, updated writer constructors to accept shared dictionary/log contexts or be no-arg, refactored virtual signatures to trailing-return style; introduced Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
davidlion
changed the title
clp_s/Schema.hpp and clp_s/ColumnWriter.hpp; Remove unused ID field from BaseColumnWriter.clp_s/Schema.hpp and clp_s/ColumnWriter.hpp; Remove unused ID field from BaseColumnWriter.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/core/src/clp_s/Schema.hpp`:
- Around line 125-129: The bounds check in Schema::get_view is overflow-prone
because i + size can wrap; replace it with a safe check such as ensuring size <=
m_schema.size() and i <= m_schema.size() - size (or equivalently i <=
m_schema.size() && size <= m_schema.size() - i) and throw OperationFailed when
the check fails; also construct the span with an explicit template parameter
(use std::span<Id>{m_schema}.subspan(i, size)) to match the style on line 117
and ensure type consistency with subspan.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/core/src/clp_s/ArchiveWriter.cppcomponents/core/src/clp_s/ColumnWriter.hppcomponents/core/src/clp_s/Schema.hpptaskfiles/lint.yaml
💤 Files with no reviewable changes (1)
- taskfiles/lint.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/core/src/clp_s/ColumnWriter.hpp`:
- Around line 205-207: The single-argument constructor
VariableStringColumnWriter(std::shared_ptr<VariableDictionaryWriter> var_dict)
should be marked explicit to prevent unintended implicit conversions; update the
VariableStringColumnWriter constructor declaration/definition to add the
explicit specifier (matching the change made for DictionaryFloatColumnWriter) so
the constructor reads as an explicit single-argument constructor.
- Around line 112-114: The single-argument constructor
DictionaryFloatColumnWriter(std::shared_ptr<VariableDictionaryWriter> var_dict)
should be marked explicit to prevent unintended implicit conversions; update the
constructor declaration/definition for DictionaryFloatColumnWriter to add the
explicit keyword (keeping the same parameter and move semantics for m_var_dict)
so the class no longer allows implicit conversions from
std::shared_ptr<VariableDictionaryWriter>.
In `@components/core/src/clp_s/Schema.hpp`:
- Around line 125-130: The bounds check in get_view is inverted and can
underflow; change the guard so it throws when the requested range is invalid by
checking that size is not greater than m_schema.size() and that i + size does
not exceed m_schema.size() (e.g. use a safe check like "if (size >
m_schema.size() || i > m_schema.size() - size) throw
OperationFailed(ErrorCodeOutOfBounds, ...)" or equivalently "if (i + size >
m_schema.size())" using a safe pre-check), keeping the throw of
OperationFailed(ErrorCodeOutOfBounds, __FILENAME__, __LINE__) and then return
the subspan from m_schema as before.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/core/src/clp_s/Schema.hpp`:
- Around line 66-67: Ensure m_num_ordered cannot exceed the schema size before
creating the ordered view: validate and clamp or reject the value set by
set_num_ordered(size_t) (or perform the check right before creating the ordered
span in the code that uses m_num_ordered), comparing m_num_ordered against
m_schema.size(); if it is larger, either reduce it to m_schema.size() or
return/throw an error so the span creation that uses m_num_ordered cannot go out
of bounds (references: set_num_ordered, m_num_ordered, m_schema, and the
ordered-span creation code that reads m_num_ordered).
Comment on lines
+66
to
67
| auto set_num_ordered(size_t num_ordered) -> void { m_num_ordered = num_ordered; } | ||
|
|
Contributor
There was a problem hiding this comment.
Validate ordered-length before creating the ordered view.
Line 66 allows any m_num_ordered, but Line 117 uses it directly as span length. If it exceeds m_schema.size(), downstream access can go out of bounds.
Suggested fix
- [[nodiscard]] auto get_ordered_schema_view() -> std::span<Id> {
- return std::span<Id>{m_schema.data(), m_num_ordered};
- }
+ [[nodiscard]] auto get_ordered_schema_view() -> std::span<Id> {
+ if (m_num_ordered > m_schema.size()) {
+ throw OperationFailed(ErrorCodeOutOfBounds, __FILENAME__, __LINE__);
+ }
+ return std::span<Id>{m_schema.data(), m_num_ordered};
+ }Also applies to: 116-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/core/src/clp_s/Schema.hpp` around lines 66 - 67, Ensure
m_num_ordered cannot exceed the schema size before creating the ordered view:
validate and clamp or reject the value set by set_num_ordered(size_t) (or
perform the check right before creating the ordered span in the code that uses
m_num_ordered), comparing m_num_ordered against m_schema.size(); if it is
larger, either reduce it to m_schema.size() or return/throw an error so the span
creation that uses m_num_ordered cannot go out of bounds (references:
set_num_ordered, m_num_ordered, m_schema, and the ordered-span creation code
that reads m_num_ordered).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
As the title says. These changes have no functional impact.
Part of the changes are to add in class organization comments that have been loosely applied up to this point in the code base. These comments are standardized in our guideline by y-scope/yscope-docs#53.
Checklist
breaking change.
Validation performed
CI passing.
Summary by CodeRabbit
Refactor
Chores