Fix C++ style casts #19

springmeyer · 2017-10-12T18:27:06Z

Fixes the compiler warnings/errors that arose from #18

Context

We should be using C++ style casts instead of C style casts. While C style casts allow for multiple types of casting to happen in one, C++ style casts require you to be explicit.

Previous to this PR we were casing a const char * into a unsigned char. So both a reinterpret cast (to go from char to unsigned char is needed) and a const_cast to drop the const.

After studying the code I realized we don't want to do the const_cast - we'd be removing the const from data that truly is const and should not be modified, which is bad practice.

Then I noticed that zlib has a ZLIB_CONST define that can be turned on to allow for const data to be passed in. This is ideal so that we don't need to cast it away.

I noticed it can be enabled here: https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zconf.h#L234

Which makes the z_const define non-blank. So, this PR moves to C++ style casts and avoids the need for a const cast.

It looks like this will mean that the minimum zlib version we can support will now be zlib 1.2.5.2 per nodejs/node#9110. I think this is fine.

Refs mapbox/cpp#37

springmeyer · 2017-10-13T00:35:40Z

include/gzip.hpp

+#include <gzip/decompress.hpp>
+#include <gzip/utils.hpp>
+#include <gzip/version.hpp>


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

springmeyer · 2017-10-13T00:36:23Z

include/gzip/compress.hpp

@@ -1,8 +1,10 @@
-#include <iostream>


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

springmeyer · 2017-10-13T00:43:18Z

include/gzip/config.hpp

@@ -0,0 +1,5 @@
+#pragma once
+


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

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

springmeyer · 2017-10-13T00:44:01Z

include/gzip/decompress.hpp

@@ -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);


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.

springmeyer · 2017-10-13T00:44:33Z

include/gzip/utils.hpp

@@ -1,4 +1,7 @@
+#include <cstdlib>
+


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

mapsam

@springmeyer just left a couple questions - thanks for your in-line comments!

mapsam · 2017-10-13T16:40:29Z

include/gzip/compress.hpp

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


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

Ah, thank you for noticing - fixed in 912a7f8

mapsam · 2017-10-13T16:43:03Z

include/gzip/compress.hpp

@@ -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));


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

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.

joto · 2017-10-13T18:49:06Z

reinterpret_casting between char* and unsigned char* is fine. This is annoying, but can't lead to any problems. The C/C++ standard explicitly leaves it open whether char is the same as unsigned char or as signed char, different implementations have historically been different. In the end it doesn't matter, because we are not using this as an arithmetic type, we are not doing any calculations with this data (where there is a difference between signed and unsigned) but seeing this as basically a blob.

springmeyer · 2017-10-13T18:57:44Z

@joto - really helpful - that addresses my gaps in knowledge and concerns - thank you 🙏

Dane Springmeyer added 2 commits October 12, 2017 11:25

fix C++ style casts

bbd8eca

add needed include to utils.hpp

640a42b

springmeyer commented Oct 13, 2017

View reviewed changes

springmeyer requested a review from mapsam October 13, 2017 00:47

mapsam reviewed Oct 13, 2017

View reviewed changes

satisfy clang-format

912a7f8

springmeyer merged commit 76ec7b5 into better-default-warnings Oct 16, 2017

springmeyer mentioned this pull request Oct 16, 2017

Add more default warnings #18

Merged

springmeyer deleted the fix-casts branch October 16, 2017 16:51

springmeyer mentioned this pull request Oct 16, 2017

Excellent docs on numeric casting in C++ mapbox/cpp#38

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix C++ style casts #19

Fix C++ style casts #19

springmeyer commented Oct 12, 2017 •

edited

Loading

springmeyer Oct 13, 2017

springmeyer Oct 13, 2017

springmeyer Oct 13, 2017

GretaCB Oct 16, 2017

springmeyer Oct 13, 2017

springmeyer Oct 13, 2017

mapsam left a comment •

edited

Loading

mapsam Oct 13, 2017

springmeyer Oct 13, 2017

mapsam Oct 13, 2017

springmeyer Oct 13, 2017

joto commented Oct 13, 2017

springmeyer commented Oct 13, 2017

Fix C++ style casts #19

Fix C++ style casts #19

Conversation

springmeyer commented Oct 12, 2017 • edited Loading

Context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapsam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joto commented Oct 13, 2017

springmeyer commented Oct 13, 2017

springmeyer commented Oct 12, 2017 •

edited

Loading

mapsam left a comment •

edited

Loading