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

Fix C++ style casts #19

Merged
merged 3 commits into from
Oct 16, 2017
Merged

Fix C++ style casts #19

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbd8eca
fix C++ style casts
Oct 12, 2017
640a42b
add needed include to utils.hpp
Oct 12, 2017
912a7f8
satisfy clang-format
Oct 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions include/gzip.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
* gzip implements a standard namespace with a few available functions.
*/

#include "gzip/compress.hpp"
#include "gzip/decompress.hpp"
#include "gzip/utils.hpp"
#include "gzip/version.hpp"
#include <gzip/compress.hpp>
#include <gzip/config.hpp>
#include <gzip/decompress.hpp>
#include <gzip/utils.hpp>
#include <gzip/version.hpp>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

^^ this is unrelated to fixing casts. Rather it is best practice to always include files the same way with <> consistently.

8 changes: 5 additions & 3 deletions include/gzip/compress.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
#include <iostream>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<iostream> was uneeded, so I removed. But to keep the code compiling I needed to add <string> (turns out was being included by ).

#include <gzip/config.hpp>

// zlib
#include <zlib.h>
// std
#include <limits>
#include <string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like make format tests are failing due to alphabetical order here .. just need to move below stdexcept

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thank you for noticing - fixed in 912a7f8

#include <stdexcept>

namespace gzip {
Expand Down Expand Up @@ -31,7 +33,7 @@ std::string compress(const char* data,
{
throw std::runtime_error("deflate init failed");
}
deflate_s.next_in = (Bytef*)data;
deflate_s.next_in = reinterpret_cast<z_const Bytef*>(data);

#ifdef DEBUG
// Verify if size input will fit into unsigned int, type used for zlib's avail_in
Expand All @@ -57,7 +59,7 @@ std::string compress(const char* data,
// There is no way we see that "increase" would not fit in an unsigned int,
// hence we use static cast here to avoid -Wshorten-64-to-32 error
deflate_s.avail_out = static_cast<unsigned int>(increase);
deflate_s.next_out = (Bytef*)(output.data() + length);
deflate_s.next_out = reinterpret_cast<Bytef*>((&output[0] + length));
Copy link
Contributor

Choose a reason for hiding this comment

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

@springmeyer mind explaining why reinterpret_cast instead of static or another?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proximately because a static_cast would not compile:

/Users/dane/projects/gzip-hpp/include/gzip/compress.hpp:62:30: error: 
      static_cast from 'value_type *' (aka 'char *') to 'Bytef *'
      (aka 'unsigned char *') is not allowed
        deflate_s.next_out = static_cast<Bytef*>((&output[0] + length));
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Which means the previous C style cast was doing the equivalent of a reinterpret_cast under the hood. A reinterpret is needed because we are changing a char * to an unsigned char *. We are doing this because a char * is what is stored inside std::string per http://en.cppreference.com/w/cpp/string/basic_string. While zlib's next_out uses a Bytef pointer and a Bytef is type def'd to a Byte which is type def'd to an unsigned char at https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zconf.h#L391.

So our options are either:

  • Make the types the same
  • Cast

In this case I'm casting since that is what the previous code did. Also because the only way I can think of making the types the same is to use a std::vector<unsigned char> instead of a std::string and I think std::string is ideal since that is what has been in use.

If you want to know the difference between char * and unsigned char* and why zlib uses unsigned I'm not sure - I'd need to tag in @joto for an opinion.

// From http://www.zlib.net/zlib_how.html
// "deflate() has a return value that can indicate errors, yet we do not check it here.
// Why not? Well, it turns out that deflate() can do no wrong here."
Expand Down
5 changes: 5 additions & 0 deletions include/gzip/config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#pragma once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is to define the ZLIB_CONST in one place. Hence this new header.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea to use this for other things as well? Like WERROR, for example?

#ifndef ZLIB_CONST
#define ZLIB_CONST
#endif
7 changes: 5 additions & 2 deletions include/gzip/decompress.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
#include <gzip/config.hpp>

// zlib
#include <zlib.h>
// std
#include <limits>
#include <string>
#include <stdexcept>

namespace gzip {
Expand All @@ -25,7 +28,7 @@ std::string decompress(const char* data, std::size_t size)
{
throw std::runtime_error("inflate init failed");
}
inflate_s.next_in = (Bytef*)data;
inflate_s.next_in = reinterpret_cast<z_const Bytef*>(data);

#ifdef DEBUG
// Verify if size (long type) input will fit into unsigned int, type used for zlib's avail_in
Expand All @@ -47,7 +50,7 @@ std::string decompress(const char* data, std::size_t size)
{
output.resize(length + 2 * size);
inflate_s.avail_out = static_cast<unsigned int>(2 * size);
inflate_s.next_out = (Bytef*)(output.data() + length);
inflate_s.next_out = reinterpret_cast<Bytef*>(&output[0] + length);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

output.data() returns a const value. But in this case output.data() is the pointer we write to - so we need the non-const/mutable pointer here.

int ret = inflate(&inflate_s, Z_FINISH);
if (ret != Z_STREAM_END && ret != Z_OK && ret != Z_BUF_ERROR)
{
Expand Down
3 changes: 3 additions & 0 deletions include/gzip/utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
#include <cstdlib>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This include is needed for this header to compile on its own, since uint8_t is used.

namespace gzip {

// These live in gzip.hpp because it doesnt need to use deps.
// Otherwise, they would need to live in impl files if these methods used
// zlib structures or functions like inflate/deflate)
Expand Down