From a82b97ea1f54211fbdf21c7e77ee17e24d51fa18 Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Tue, 2 Dec 2014 14:54:21 +0100 Subject: [PATCH 01/17] Enable more warning for test builds --- test-high/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-high/Makefile b/test-high/Makefile index 48ff6d5..3f62e47 100644 --- a/test-high/Makefile +++ b/test-high/Makefile @@ -38,7 +38,7 @@ TARGETS = \ check-pcg64_oneseq check-pcg64_oneseq_once_insecure check-pcg64_unique \ check-pcg128_once_insecure check-pcg128_oneseq_once_insecure -CPPFLAGS += -I../include +CPPFLAGS += -I../include -Wall -Wextra CXXFLAGS += -std=c++11 CC = $(CXX) # Cheat so that linking uses the C++ compiler From b304ecc6c249cfc2d6568297bd9101071890a2bf Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Tue, 2 Dec 2014 15:00:23 +0100 Subject: [PATCH 02/17] Second parameter of operator>>(...) should be a reference --- include/pcg_extras.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pcg_extras.hpp b/include/pcg_extras.hpp index ec3e569..23a416f 100644 --- a/include/pcg_extras.hpp +++ b/include/pcg_extras.hpp @@ -220,7 +220,7 @@ operator<<(std::basic_ostream&out, uint8_t value) template std::basic_istream& -operator>>(std::basic_istream& in, uint8_t target) +operator>>(std::basic_istream& in, uint8_t& target) { uint32_t value = 0xdecea5edU; in >> value; From 8ad0c4b6812c39f9958af81f45c974af27a7a2d0 Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Tue, 2 Dec 2014 15:13:00 +0100 Subject: [PATCH 03/17] Remove unused copy of RNG Copying the RNG is not part of this test. It is tested in pcg-test.cpp --- test-high/pcg-test-noadvance.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test-high/pcg-test-noadvance.cpp b/test-high/pcg-test-noadvance.cpp index 4e43b67..61c414d 100644 --- a/test-high/pcg-test-noadvance.cpp +++ b/test-high/pcg-test-noadvance.cpp @@ -126,7 +126,6 @@ int main(int argc, char** argv) cout << (rng(2) ? "H" : "T"); cout << endl; - RNG rng_copy{rng}; /* Roll some dice */ printf(" Rolls:"); for (int i = 0; i < 33; ++i) From d88f914660ca5e0a782e30ee4e129b168d3101a3 Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Tue, 2 Dec 2014 15:25:38 +0100 Subject: [PATCH 04/17] Enable extra warnings for clang Because this is a test build all warnings (except compat) are helpful. They may be used to increase the quality of the library. --- test-high/Makefile | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test-high/Makefile b/test-high/Makefile index 3f62e47..1604b02 100644 --- a/test-high/Makefile +++ b/test-high/Makefile @@ -42,6 +42,11 @@ CPPFLAGS += -I../include -Wall -Wextra CXXFLAGS += -std=c++11 CC = $(CXX) # Cheat so that linking uses the C++ compiler +# extra warnings +ifeq ($(CXX),clang++) + CPPFLAGS += -Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic +endif + all: $(TARGETS) test: $(TARGETS) From a88a01524c73d992e128ee00526c5fc572ced61e Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Tue, 2 Dec 2014 15:31:36 +0100 Subject: [PATCH 05/17] Replace macro calculation magic with definded checks --- include/pcg_extras.hpp | 6 +++--- test-high/pcg-test-noadvance.cpp | 2 +- test-high/pcg-test.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/pcg_extras.hpp b/include/pcg_extras.hpp index 23a416f..ddddac1 100644 --- a/include/pcg_extras.hpp +++ b/include/pcg_extras.hpp @@ -295,7 +295,7 @@ inline itype rotl(itype value, bitcount_t rot) { constexpr bitcount_t bits = sizeof(itype) * 8; constexpr bitcount_t mask = bits - 1; -#if PCG_USE_ZEROCHECK_ROTATE_IDIOM +#ifdef PCG_USE_ZEROCHECK_ROTATE_IDIOM return rot ? (value << rot) | (value >> (bits - rot)) : value; #else return (value << rot) | (value >> ((- rot) & mask)); @@ -307,7 +307,7 @@ inline itype rotr(itype value, bitcount_t rot) { constexpr bitcount_t bits = sizeof(itype) * 8; constexpr bitcount_t mask = bits - 1; -#if PCG_USE_ZEROCHECK_ROTATE_IDIOM +#ifdef PCG_USE_ZEROCHECK_ROTATE_IDIOM return rot ? (value >> rot) | (value << (bits - rot)) : value; #else return (value >> rot) | (value << ((- rot) & mask)); @@ -321,7 +321,7 @@ inline itype rotr(itype value, bitcount_t rot) * * These overloads will be preferred over the general template code above. */ -#if PCG_USE_INLINE_ASM && __GNUC__ && (__x86_64__ || __i386__) +#if defined(PCG_USE_INLINE_ASM) && defined(__GNUC__) && (defined(__x86_64__) || defined(__i386__)) inline uint8_t rotr(uint8_t value, bitcount_t rot) { diff --git a/test-high/pcg-test-noadvance.cpp b/test-high/pcg-test-noadvance.cpp index 61c414d..8ef24db 100644 --- a/test-high/pcg-test-noadvance.cpp +++ b/test-high/pcg-test-noadvance.cpp @@ -53,7 +53,7 @@ using namespace std; using pcg_extras::operator<<; -#if !PCG_EMULATED_128BIT_MATH || !AWKWARD_128BIT_CODE +#if !defined(PCG_EMULATED_128BIT_MATH) || !defined(AWKWARD_128BIT_CODE) int main(int argc, char** argv) { diff --git a/test-high/pcg-test.cpp b/test-high/pcg-test.cpp index c68f2b0..2cc3310 100644 --- a/test-high/pcg-test.cpp +++ b/test-high/pcg-test.cpp @@ -53,7 +53,7 @@ using namespace std; using pcg_extras::operator<<; -#if !PCG_EMULATED_128BIT_MATH || !AWKWARD_128BIT_CODE +#if !defined(PCG_EMULATED_128BIT_MATH) || !defined(AWKWARD_128BIT_CODE) int main(int argc, char** argv) { From 7b9787bc828091ba5a9f08405e76c3c64373d14d Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Tue, 2 Dec 2014 15:39:49 +0100 Subject: [PATCH 06/17] Do not warn about non-reproducible builds --- test-high/Makefile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test-high/Makefile b/test-high/Makefile index 1604b02..5ca27e7 100644 --- a/test-high/Makefile +++ b/test-high/Makefile @@ -44,7 +44,11 @@ CC = $(CXX) # Cheat so that linking uses the C++ compiler # extra warnings ifeq ($(CXX),clang++) - CPPFLAGS += -Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic + CPPFLAGS += \ + -Weverything \ + -Wno-c++98-compat \ + -Wno-c++98-compat-pedantic \ + -Wno-date-time endif all: $(TARGETS) From 6c929bdf6b16b30ab8075cc30c06fb43c08d50ed Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Tue, 2 Dec 2014 16:00:59 +0100 Subject: [PATCH 07/17] Do not warn about unused macros --- test-high/Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test-high/Makefile b/test-high/Makefile index 5ca27e7..7af5d5b 100644 --- a/test-high/Makefile +++ b/test-high/Makefile @@ -48,7 +48,8 @@ ifeq ($(CXX),clang++) -Weverything \ -Wno-c++98-compat \ -Wno-c++98-compat-pedantic \ - -Wno-date-time + -Wno-date-time \ + -Wno-unused-macros endif all: $(TARGETS) From 0e0e050939b009d721e1c4db94b71471f8b287a2 Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Tue, 2 Dec 2014 16:02:01 +0100 Subject: [PATCH 08/17] Replace some C pointer magic with idiomatic C++ code --- include/pcg_extras.hpp | 8 ++++---- include/pcg_random.hpp | 7 ++++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/include/pcg_extras.hpp b/include/pcg_extras.hpp index ddddac1..c3ff9d0 100644 --- a/include/pcg_extras.hpp +++ b/include/pcg_extras.hpp @@ -483,10 +483,10 @@ void generate_to_impl(SeedSeq&& generator, DestIter dest, generator.generate(buffer, buffer+FROM_ELEMS); uneven_copy(buffer, dest, dest+size); } else { - uint32_t* buffer = (uint32_t*) malloc(GEN_SIZE * FROM_ELEMS); + uint32_t* buffer = new uint32_t[FROM_ELEMS]; generator.generate(buffer, buffer+FROM_ELEMS); uneven_copy(buffer, dest, dest+size); - free(buffer); + delete[] buffer; } } @@ -620,11 +620,11 @@ std::ostream& operator<<(std::ostream& out, printable_typename) { const char *implementation_typename = typeid(T).name(); #ifdef __GNUC__ int status; - const char* pretty_name = + char* pretty_name = abi::__cxa_demangle(implementation_typename, NULL, NULL, &status); if (status == 0) out << pretty_name; - free((void*) pretty_name); + free(static_cast(pretty_name)); if (status == 0) return out; #endif diff --git a/include/pcg_random.hpp b/include/pcg_random.hpp index 59bc480..f3d7c6d 100644 --- a/include/pcg_random.hpp +++ b/include/pcg_random.hpp @@ -75,6 +75,7 @@ #ifndef PCG_RAND_HPP_INCLUDED #define PCG_RAND_HPP_INCLUDED 1 +#include #include #include #include @@ -82,6 +83,7 @@ #include #include #include +#include #include #include #include @@ -1360,7 +1362,10 @@ bool operator==(const extended(lhs); auto& base_rhs = static_cast(rhs); return base_lhs == base_rhs - && !memcmp((void*) lhs.data_, (void*) rhs.data_, sizeof(lhs.data_)); + && !std::equal( + std::begin(lhs.data_), std::end(lhs.data_), + std::begin(rhs.data_) + ); } template Date: Tue, 2 Dec 2014 16:06:59 +0100 Subject: [PATCH 09/17] Use modulo operator instead of manual calculation --- include/pcg_extras.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pcg_extras.hpp b/include/pcg_extras.hpp index c3ff9d0..835745f 100644 --- a/include/pcg_extras.hpp +++ b/include/pcg_extras.hpp @@ -156,7 +156,7 @@ operator<<(std::basic_ostream& out, pcg128_t value) constexpr auto BASE = pcg128_t(10ULL); do { auto div = value / BASE; - auto mod = uint32_t(value - (div * BASE)); + auto mod = char(value % BASE); *(--pos) = '0' + mod; value = div; } while(value != pcg128_t(0ULL)); From 6a250cd85fb2a9a93e6c66532f73620750fa686f Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Tue, 2 Dec 2014 16:20:44 +0100 Subject: [PATCH 10/17] Clarify some type conversions --- include/pcg_extras.hpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/include/pcg_extras.hpp b/include/pcg_extras.hpp index 835745f..69e3888 100644 --- a/include/pcg_extras.hpp +++ b/include/pcg_extras.hpp @@ -531,13 +531,17 @@ template void shuffle(Iter from, Iter to, RandType&& rng) { typedef typename std::iterator_traits::difference_type delta_t; + typedef typename std::remove_reference::type::result_type result_t; auto count = to - from; while (count > 1) { - delta_t chosen(bounded_rand(rng, count)); + delta_t chosen = static_cast(bounded_rand( + rng, + static_cast(count) + )); --count; --to; using std::swap; - swap(*(from+chosen), *to); + swap(*(std::next(from, chosen)), *to); } } From 8450f78011dd050db542e44344a9b2b5156d9b57 Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Wed, 3 Dec 2014 19:24:43 +0100 Subject: [PATCH 11/17] Revert "Replace macro calculation magic with definded checks" This reverts commit a88a01524c73d992e128ee00526c5fc572ced61e. --- include/pcg_extras.hpp | 6 +++--- test-high/pcg-test-noadvance.cpp | 2 +- test-high/pcg-test.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/pcg_extras.hpp b/include/pcg_extras.hpp index 69e3888..6857a11 100644 --- a/include/pcg_extras.hpp +++ b/include/pcg_extras.hpp @@ -295,7 +295,7 @@ inline itype rotl(itype value, bitcount_t rot) { constexpr bitcount_t bits = sizeof(itype) * 8; constexpr bitcount_t mask = bits - 1; -#ifdef PCG_USE_ZEROCHECK_ROTATE_IDIOM +#if PCG_USE_ZEROCHECK_ROTATE_IDIOM return rot ? (value << rot) | (value >> (bits - rot)) : value; #else return (value << rot) | (value >> ((- rot) & mask)); @@ -307,7 +307,7 @@ inline itype rotr(itype value, bitcount_t rot) { constexpr bitcount_t bits = sizeof(itype) * 8; constexpr bitcount_t mask = bits - 1; -#ifdef PCG_USE_ZEROCHECK_ROTATE_IDIOM +#if PCG_USE_ZEROCHECK_ROTATE_IDIOM return rot ? (value >> rot) | (value << (bits - rot)) : value; #else return (value >> rot) | (value << ((- rot) & mask)); @@ -321,7 +321,7 @@ inline itype rotr(itype value, bitcount_t rot) * * These overloads will be preferred over the general template code above. */ -#if defined(PCG_USE_INLINE_ASM) && defined(__GNUC__) && (defined(__x86_64__) || defined(__i386__)) +#if PCG_USE_INLINE_ASM && __GNUC__ && (__x86_64__ || __i386__) inline uint8_t rotr(uint8_t value, bitcount_t rot) { diff --git a/test-high/pcg-test-noadvance.cpp b/test-high/pcg-test-noadvance.cpp index 8ef24db..61c414d 100644 --- a/test-high/pcg-test-noadvance.cpp +++ b/test-high/pcg-test-noadvance.cpp @@ -53,7 +53,7 @@ using namespace std; using pcg_extras::operator<<; -#if !defined(PCG_EMULATED_128BIT_MATH) || !defined(AWKWARD_128BIT_CODE) +#if !PCG_EMULATED_128BIT_MATH || !AWKWARD_128BIT_CODE int main(int argc, char** argv) { diff --git a/test-high/pcg-test.cpp b/test-high/pcg-test.cpp index 2cc3310..c68f2b0 100644 --- a/test-high/pcg-test.cpp +++ b/test-high/pcg-test.cpp @@ -53,7 +53,7 @@ using namespace std; using pcg_extras::operator<<; -#if !defined(PCG_EMULATED_128BIT_MATH) || !defined(AWKWARD_128BIT_CODE) +#if !PCG_EMULATED_128BIT_MATH || !AWKWARD_128BIT_CODE int main(int argc, char** argv) { From 037f96a6f52cec6e7addf67ac56dc0db6659203f Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Wed, 3 Dec 2014 19:28:31 +0100 Subject: [PATCH 12/17] Replace d88f914660ca5e0a782e30ee4e129b1 with a more portable version This should work on all versions of make. Thanks to stackoverflow for this idea: https://stackoverflow.com/a/1328441/1718219 --- test-high/Makefile | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/test-high/Makefile b/test-high/Makefile index 7af5d5b..00e446d 100644 --- a/test-high/Makefile +++ b/test-high/Makefile @@ -38,20 +38,18 @@ TARGETS = \ check-pcg64_oneseq check-pcg64_oneseq_once_insecure check-pcg64_unique \ check-pcg128_once_insecure check-pcg128_oneseq_once_insecure -CPPFLAGS += -I../include -Wall -Wextra +# special flags for some compilers +CPPFLAGS_clang++ += \ + -Weverything \ + -Wno-c++98-compat \ + -Wno-c++98-compat-pedantic \ + -Wno-date-time \ + -Wno-unused-macros + +CPPFLAGS += -I../include -Wall -Wextra $(CPPFLAGS_$(CXX)) CXXFLAGS += -std=c++11 CC = $(CXX) # Cheat so that linking uses the C++ compiler -# extra warnings -ifeq ($(CXX),clang++) - CPPFLAGS += \ - -Weverything \ - -Wno-c++98-compat \ - -Wno-c++98-compat-pedantic \ - -Wno-date-time \ - -Wno-unused-macros -endif - all: $(TARGETS) test: $(TARGETS) From ef0835f4998ccd7e8e7fde8a205e16dcc2aba7ae Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Thu, 4 Dec 2014 19:55:27 +0100 Subject: [PATCH 13/17] Go back to malloc and free This change was discussed as part of 0e0e050939b009d721e1c4db94b71471f8b287a2 --- include/pcg_extras.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pcg_extras.hpp b/include/pcg_extras.hpp index 6857a11..6916d77 100644 --- a/include/pcg_extras.hpp +++ b/include/pcg_extras.hpp @@ -483,10 +483,10 @@ void generate_to_impl(SeedSeq&& generator, DestIter dest, generator.generate(buffer, buffer+FROM_ELEMS); uneven_copy(buffer, dest, dest+size); } else { - uint32_t* buffer = new uint32_t[FROM_ELEMS]; + uint32_t* buffer = static_cast(malloc(GEN_SIZE * FROM_ELEMS)); generator.generate(buffer, buffer+FROM_ELEMS); uneven_copy(buffer, dest, dest+size); - delete[] buffer; + free(static_cast(buffer)); } } From 8c1e5366d411655374d719a5f66203511fec92be Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Thu, 4 Dec 2014 19:58:16 +0100 Subject: [PATCH 14/17] Silence some clang warnings See discussion of a88a01524c73d992e128ee00526c5fc572ced61e --- test-high/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/test-high/Makefile b/test-high/Makefile index 00e446d..3d11c2f 100644 --- a/test-high/Makefile +++ b/test-high/Makefile @@ -44,6 +44,7 @@ CPPFLAGS_clang++ += \ -Wno-c++98-compat \ -Wno-c++98-compat-pedantic \ -Wno-date-time \ + -Wno-undef \ -Wno-unused-macros CPPFLAGS += -I../include -Wall -Wextra $(CPPFLAGS_$(CXX)) From 697298b01afe4b1a7fab96a08d49027111fd5dfd Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Thu, 4 Dec 2014 20:01:46 +0100 Subject: [PATCH 15/17] Fix issues of 6a250cd85fb2a9a93e6c66532f73620750fa686f --- include/pcg_extras.hpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/include/pcg_extras.hpp b/include/pcg_extras.hpp index 6916d77..2ae7e03 100644 --- a/include/pcg_extras.hpp +++ b/include/pcg_extras.hpp @@ -534,14 +534,11 @@ void shuffle(Iter from, Iter to, RandType&& rng) typedef typename std::remove_reference::type::result_type result_t; auto count = to - from; while (count > 1) { - delta_t chosen = static_cast(bounded_rand( - rng, - static_cast(count) - )); + delta_t chosen = delta_t(bounded_rand(rng, result_t(count))); --count; --to; using std::swap; - swap(*(std::next(from, chosen)), *to); + swap(*(from + chosen), *to); } } From 70df571feb282639dd29ac6b590c7ddb13c6e865 Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Thu, 4 Dec 2014 20:16:24 +0100 Subject: [PATCH 16/17] Clarify remaining sign changes --- include/pcg_random.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pcg_random.hpp b/include/pcg_random.hpp index f3d7c6d..4550b3f 100644 --- a/include/pcg_random.hpp +++ b/include/pcg_random.hpp @@ -323,7 +323,7 @@ class specific_stream { specific_stream() = default; specific_stream(itype specific_seq) - : inc_((specific_seq << 1) | itype(1U)) + : inc_(itype(specific_seq << 1) | itype(1U)) { // Nothing (else) to do. } @@ -382,7 +382,7 @@ class engine : protected output_mixin, static constexpr result_type max() { - return ~result_type(0UL); + return result_type(~result_type(0UL)); } protected: @@ -424,7 +424,7 @@ class engine : protected output_mixin, static itype distance(itype cur_state, itype newstate, itype cur_mult, itype cur_plus, itype mask = ~itype(0U)); - itype distance(itype newstate, itype mask = ~itype(0U)) const + itype distance(itype newstate, itype mask = itype(~itype(0U))) const { return distance(state_, newstate, multiplier(), increment(), mask); } From f9d15e1da0914ea25f5ab8dd6d403d3af0201e8e Mon Sep 17 00:00:00 2001 From: Marco Neumann Date: Thu, 4 Dec 2014 20:29:18 +0100 Subject: [PATCH 17/17] Silence "variable unitialized" warnings This changes doesn't really impact performance but enable us to be more strict about this type of warnings and to discover bugs early. --- include/pcg_extras.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pcg_extras.hpp b/include/pcg_extras.hpp index 2ae7e03..d4c710d 100644 --- a/include/pcg_extras.hpp +++ b/include/pcg_extras.hpp @@ -135,7 +135,7 @@ operator<<(std::basic_ostream& out, pcg128_t value) } if (highpart != 0 || desired_width > 16) out << highpart; - CharT oldfill; + CharT oldfill = '\0'; if (highpart != 0) { out.width(16); oldfill = out.fill('0'); @@ -393,7 +393,7 @@ SrcIter uneven_copy_impl( constexpr bitcount_t SCALE = SRC_SIZE / DEST_SIZE; size_t count = 0; - src_t value; + src_t value = 0; while (dest_first != dest_last) { if ((count++ % SCALE) == 0)