Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[compression] Follow ROS2 style conventions better and throw eagerly #245

Merged
merged 3 commits into from
Jan 7, 2020
Merged

[compression] Follow ROS2 style conventions better and throw eagerly #245

merged 3 commits into from
Jan 7, 2020

Conversation

zmichaels11
Copy link
Contributor

Changes

  • Renamed check_frame_content to throw_on_invalid_frame_content to match what its actually doing.
  • [ZstdCompressor] Throw an exception if file size is 0 when opening a file for compression.
  • [ZstdDecompressor] Throw an exception if file size is 0 when opening a file for decompression.
  • [ZstdCompressor] Document private methods.
  • Align to ROS2 style guide better:
  • Use brace-initialization when possible.

Signed-off-by: Zachary Michaels zmichaels11@gmail.com

@zmichaels11 zmichaels11 mentioned this pull request Jan 6, 2020
Merged
2 tasks
@piraka9011 piraka9011 requested review from piraka9011 and Karsten1987 and removed request for piraka9011 January 6, 2020 18:25
piraka9011
piraka9011 approved these changes Jan 6, 2020
View reviewed changes
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zmichaels11 zmichaels11 marked this pull request as ready for review January 6, 2020 19:14
Karsten1987
Karsten1987 approved these changes Jan 6, 2020
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm me green CI

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp Show resolved Hide resolved
Karsten1987
Karsten1987 reviewed Jan 6, 2020
View reviewed changes
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp
@@ -30,10 +30,17 @@ namespace
// - Increase the time taken to compress
// - Decrease the size of the compressed data
// Setting to zero uses Zstd's default value of 3.
constexpr const int DEFAULT_ZSTD_COMPRESSION_LEVEL = 1;
constexpr const int kDefaultZstdCompressionLevel = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that I really matters, but given that the style guide recommends g_ followed by underscore, I think we could do the same for k_ for constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I interpreted the variables as being constants and not globals since its scope is restricted to the file due to being in an anonymous namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding also is that global constants are kCamelCase and global variables are g_snake_case.

@zmichaels11
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/e3c4923478709c998bdd329516591c7c/raw/703da413d1b2fe45370ef62eb486619bf52694a3/ros2.repos
BUILD args: --packages-up-to rosbag2_compression
TEST args: --packages-select rosbag2_compression
Job: ci_launcher

@dabonnie
Copy link
Contributor

dabonnie commented Jan 6, 2020

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/e3c4923478709c998bdd329516591c7c/raw/703da413d1b2fe45370ef62eb486619bf52694a3/ros2.repos
BUILD args: --packages-up-to rosbag2_compression
TEST args: --packages-select rosbag2_compression
Job: ci_launcher

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@zmichaels11
Copy link
Contributor Author

Windows build failed due to buildfarm issues; rerun:

  • Windows Build Status

@piraka9011
Copy link
Contributor

@zmichaels11 Suggest to merge this after #241

@zmichaels11
Follow style conventions better and throw eagerly
de675fe 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Use constant naming convention in ZstdCompressor
f5769bc 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Remove redundant whitespace
3cd5217 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Jan 6, 2020 •
edited
Loading

Rebased on master.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Buildfarm issues; rebuilding

  • Windows Build Status

@zmichaels11 zmichaels11 merged commit 57a0b9a into ros2:master Jan 7, 2020
@zmichaels11 zmichaels11 deleted the decompression/robustness branch January 7, 2020 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@piraka9011 piraka9011 piraka9011 approved these changes

@dabonnie dabonnie dabonnie approved these changes

@mm318 mm318 Awaiting requested review from mm318

@emersonknapp emersonknapp Awaiting requested review from emersonknapp

@prajakta-gokhale prajakta-gokhale Awaiting requested review from prajakta-gokhale

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zmichaels11 @dabonnie @piraka9011 @Karsten1987 @mm318