Repository navigation
kvs: apply C++ idiom and class layout rules - #566
Merged
vinodreddy-g merged 1 commit intoOct 28, 2026
Merged
Conversation
arkjedrz
requested review from
PandaeDo,
antonkri,
umaucher and
vinodreddy-g
as code owners
October 22, 2026 12:56
Contributor
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
Contributor
|
Documentation preview for this pull request is available at: |
arkjedrz
force-pushed
the
arkjedrz_style-04-idioms
branch
from
October 23, 2026 06:58
0de5fe2 to
afded22
Compare
arkjedrz
force-pushed
the
arkjedrz_style-04-idioms
branch
from
October 27, 2026 11:27
afded22 to
ec3feca
Compare
arkjedrz
force-pushed
the
arkjedrz_style-04-idioms
branch
from
October 28, 2026 07:28
ec3feca to
e086807
Compare
arkjedrz
had a problem deploying
to
workflow-approval
October 28, 2026 07:28 — with 
GitHub Actions
Error
GitHub Actions
Error
arkjedrz
had a problem deploying
to
workflow-approval
October 28, 2026 07:28 — with
GitHub Actions
Error
GitHub Actions
Error
Ahmed-Elsaka-JC
suggested changes
Oct 28, 2026
| return total_size; | ||
| } | ||
|
|
||
| score::Result<void> Kvs::write_temp_file(const score::filesystem::Path& path, |
Contributor
There was a problem hiding this comment.
[non blocking] when you refactor function keep it in same location , do not update function adn change it's location in oneshot that makes review not easy.
Contributor
Author
There was a problem hiding this comment.
It's not random, it's part of the change.
- Order class sections and members as the style guide requires.
| return {}; | ||
| } | ||
|
|
||
| score::Result<score::filesystem::Path> Kvs::KvsFilePath(const SnapshotId& snapshot_id) const noexcept |
Contributor
There was a problem hiding this comment.
this function returns file name and old name was get_kvs_filename why did you change it ??
Contributor
Author
There was a problem hiding this comment.
- Alignment with Rust implementation.
- No rules on that in the guidelines.
- It's not really a getter, as those methods construct the path.
Contributor
There was a problem hiding this comment.
this function returns file name , not file path , i would recommend to adjust rust name instead of c++ name to misleading name
Contributor
Author
There was a problem hiding this comment.
Both KvsFilePath and HashFilePath work the same way.
Functions return file paths, that are created by taking filename_prefix_, then appending snapshot ID, then extension.
If such file doesn't exist, then an error is returned.
filename_prefix_ is set in Kvs::Open, and it's a working directory with kvs_, instance ID and a _ appended.
With Rust there are two separate methods: *_file_path and *_file_name.
*_file_pathreturns full path (working dir + file name).*_file_namereturns file name (sokvs_<instance_id>_<snapshot_id>.<extension>.
| } | ||
|
|
||
| for (const char* extension : file_extensions) | ||
| score::Result<score::filesystem::Path> Kvs::HashFilePath(const SnapshotId& snapshot_id) const noexcept |
Contributor
There was a problem hiding this comment.
why did you rename it form get_hash_filename to HathFilePath ??
Contributor
Author
There was a problem hiding this comment.
- Alignment with Rust implementation.
- No rules on that in the guidelines.
- It's not really a getter, as those methods construct the path.
Contributor
There was a problem hiding this comment.
again , this function returns hash file name not full path .
arkjedrz
force-pushed
the
arkjedrz_style-04-idioms
branch
from
October 28, 2026 12:43
e086807 to
5179e8f
Compare
- Brace-initialize declarations. Count/value constructors and initializer_list types keep parentheses. - Declare one variable per declaration. - Order class sections and members as the style guide requires. Move the KvsValue::ValueVariant alias next to Array and Object. - Initialize InstanceId, SnapshotId and KvsValue members in the initializer list. - Define members in .cpp files in declaration order and group file-local helpers at the top. - Name repeated file suffixes and the hash size. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
arkjedrz
force-pushed
the
arkjedrz_style-04-idioms
branch
from
October 28, 2026 15:00
5179e8f to
980a361
Compare
vinodreddy-g
approved these changes
Oct 28, 2026
This branch was successfully deployed
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
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.
Merge #564 first.
Relates to #556