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.

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

Change validation functions to accept output type of ZSTD_getFrameContentSize #285

Conversation

zmichaels11
Copy link
Contributor

Addresses the same issue to #282.
The root cause of the problem was an implicit conversion from unsigned long long to size_t (same on x86_64 platforms)

Changes

  • Accept unsigned long long directly. This is the return type of ZSTD_getFrameContentSize

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

…tentSize

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 marked this pull request as ready for review February 4, 2020 21:32
@zmichaels11
Copy link
Contributor Author

please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/96d24f9df801fdf5cba371185dfc0af1/raw/a7f0a443519a12a55fca1a4ed05fa7168e085a43/ros2.repos
BUILD args: --packages-up-to rosbag2_compression
TEST args: --packages-select rosbag2_compression
Job: ci_launche

@zmichaels11
Copy link
Contributor Author

  • Linux armhf Build Status

Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

Could we avoid the NOLINT here?

@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Feb 4, 2020

Could we avoid the NOLINT here?

The lint warning is from using unsigned long long. I opted to use NOLINT in favor of using a sized-type because its the return value of ZSTD_getFrameContentSize which is directly passed into the functions.

Edit:
Would

/**
 * Checks compression_result and throws a runtime_error if there was a ZSTD error.
 * \param compression_result is the return value of ZSTD_decompress.
 */
void throw_on_zstd_error(
  const decltype(ZSTD_decompress(nullptr, 0, nullptr, 0)) compression_result
)

Be acceptable?

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Feb 4, 2020

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

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Nice call with the decltype!

@zmichaels11
Copy link
Contributor Author

Nice call with the decltype!

I think you missed a hallway discussion with Thomas; paraphrasing:
decltype should be avoided since its deep magic. This current commit is a compromise from a previous (unpublished) iteration and uses type aliasing to comment via code that decltype yields the return type of the function.

Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

LGTM, consider adding a comment on top of the decltype just to explain why we have this here.

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11 zmichaels11 merged commit ad8b530 into ros2:master Feb 5, 2020
@zmichaels11 zmichaels11 deleted the zmichaels11/compression/fix-implicit-conversion branch February 5, 2020 00:23
@@ -34,6 +34,8 @@ constexpr const int kDefaultZstdCompressionLevel = 1;

// String constant used to identify ZstdCompressor.
constexpr const char kCompressionIdentifier[] = "zstd";
// Used as a parameter type in a function that accepts the output of ZSTD_compress.
using ZstdCompressReturnType = decltype(ZSTD_compress(nullptr, 0, nullptr, 0, 0));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use https://en.cppreference.com/w/cpp/types/result_of instead of making a function call.

Copy link
Contributor Author

@zmichaels11 zmichaels11 Feb 5, 2020

Choose a reason for hiding this comment

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

decltype actually doesn't make a function call! (I'll update with the document stating this)

Copy link
Member

Choose a reason for hiding this comment

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

Never mind. decltype doesn't invoke anything, but result_of is better in this situation because you won't need to write the dummy values to "pass" into the ZSTD_compress() function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree, unfortunately I merged this before you commented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants