From b2c14972de265b3784d9828e02fb1395259aeaab Mon Sep 17 00:00:00 2001 From: Mickey Rose Date: Fri, 29 Jan 2016 21:04:47 +0100 Subject: [PATCH 1/4] remove explicitly-defaulted constructors and assignment operators ... ... where they should be implicitly defined by the compiler --- include/mapnik/geometry.hpp | 19 +++---------------- include/mapnik/image.hpp | 3 --- include/mapnik/image_impl.hpp | 8 -------- include/mapnik/image_null.hpp | 3 --- include/mapnik/image_view.hpp | 4 ---- include/mapnik/image_view_impl.hpp | 19 ------------------- include/mapnik/marker.hpp | 21 ++++----------------- include/mapnik/vertex.hpp | 17 ----------------- 8 files changed, 7 insertions(+), 87 deletions(-) diff --git a/include/mapnik/geometry.hpp b/include/mapnik/geometry.hpp index 3766ebf23..7063685d7 100644 --- a/include/mapnik/geometry.hpp +++ b/include/mapnik/geometry.hpp @@ -45,9 +45,6 @@ struct point point(mapnik::coord const& c) : x(c.x), y(c.y) {} - point(point const& other) = default; - point(point && other) noexcept = default; - point & operator=(point const& other) = default; friend inline bool operator== (point const& a, point const& b) { return a.x == b.x && a.y == b.y; @@ -65,12 +62,8 @@ template struct line_string : std::vector > { line_string() = default; - line_string (std::size_t size) + explicit line_string(std::size_t size) : std::vector >(size) {} - line_string (line_string && other) = default ; - line_string& operator=(line_string &&) = default; - line_string (line_string const& ) = default; - line_string& operator=(line_string const&) = default; inline std::size_t num_points() const { return std::vector>::size(); } inline void add_coord(T x, T y) { std::vector>::template emplace_back(x,y);} }; @@ -79,17 +72,12 @@ template struct linear_ring : line_string { linear_ring() = default; - linear_ring(std::size_t size) + explicit linear_ring(std::size_t size) : line_string(size) {} - linear_ring (linear_ring && other) = default ; - linear_ring& operator=(linear_ring &&) = default; linear_ring(line_string && other) - : line_string(other) {} - linear_ring (linear_ring const& ) = default; + : line_string(std::move(other)) {} linear_ring(line_string const& other) : line_string(other) {} - linear_ring& operator=(linear_ring const&) = default; - }; template @@ -102,7 +90,6 @@ struct polygon using rings_container = InteriorRings; rings_container interior_rings; - polygon() = default; inline void set_exterior_ring(linear_ring && ring) { exterior_ring = std::move(ring); diff --git a/include/mapnik/image.hpp b/include/mapnik/image.hpp index 9ebdfc3b2..13b21c856 100644 --- a/include/mapnik/image.hpp +++ b/include/mapnik/image.hpp @@ -54,9 +54,6 @@ template struct image_dimensions { image_dimensions(int width, int height); - image_dimensions(image_dimensions const& other) = default; - image_dimensions(image_dimensions && other) = default; - image_dimensions& operator= (image_dimensions rhs); std::size_t width() const; std::size_t height() const; private: diff --git a/include/mapnik/image_impl.hpp b/include/mapnik/image_impl.hpp index 732e7ebe6..c756547b0 100644 --- a/include/mapnik/image_impl.hpp +++ b/include/mapnik/image_impl.hpp @@ -43,14 +43,6 @@ image_dimensions::image_dimensions(int width, int height) if (height < 0 || static_cast(height) > max_size) throw std::runtime_error("Invalid height for image dimensions requested"); } -template -image_dimensions& image_dimensions::operator= (image_dimensions rhs) -{ - std::swap(width_, rhs.width_); - std::swap(height_, rhs.height_); - return *this; -} - template std::size_t image_dimensions::width() const { diff --git a/include/mapnik/image_null.hpp b/include/mapnik/image_null.hpp index 2fc4e32c5..5a698bbc9 100644 --- a/include/mapnik/image_null.hpp +++ b/include/mapnik/image_null.hpp @@ -48,9 +48,6 @@ public: bool /*initialize*/ = true, bool /*premultiplied*/ = false, bool /*painted*/ = false) {} - image(image const&) {} - image(image &&) noexcept {} - image& operator=(image) { return *this; } bool operator==(image const&) const { return true; } bool operator<(image const&) const { return false; } diff --git a/include/mapnik/image_view.hpp b/include/mapnik/image_view.hpp index c2f023a30..846da6691 100644 --- a/include/mapnik/image_view.hpp +++ b/include/mapnik/image_view.hpp @@ -37,11 +37,7 @@ public: static constexpr std::size_t pixel_size = sizeof(pixel_type); image_view(std::size_t x, std::size_t y, std::size_t width, std::size_t height, T const& data); - ~image_view(); - image_view(image_view const& rhs); - image_view(image_view && other) noexcept; - image_view& operator=(image_view rhs) = delete; bool operator==(image_view const& rhs) const; bool operator<(image_view const& rhs) const; diff --git a/include/mapnik/image_view_impl.hpp b/include/mapnik/image_view_impl.hpp index 89ae15182..14032042a 100644 --- a/include/mapnik/image_view_impl.hpp +++ b/include/mapnik/image_view_impl.hpp @@ -42,25 +42,6 @@ image_view::image_view(std::size_t x, std::size_t y, std::size_t width, std:: if (y_ + height_ > data_.height()) height_ = data_.height() - y_; } -template -image_view::~image_view() {} - -template -image_view::image_view(image_view const& rhs) - : x_(rhs.x_), - y_(rhs.y_), - width_(rhs.width_), - height_(rhs.height_), - data_(rhs.data_) {} - -template -image_view::image_view(image_view && other) noexcept - : x_(std::move(other.x_)), - y_(std::move(other.y_)), - width_(std::move(other.width_)), - height_(std::move(other.height_)), - data_(std::move(other.data_)) {} - template bool image_view::operator==(image_view const& rhs) const { diff --git a/include/mapnik/marker.hpp b/include/mapnik/marker.hpp index bde8d65a2..ff7ac7028 100644 --- a/include/mapnik/marker.hpp +++ b/include/mapnik/marker.hpp @@ -58,18 +58,12 @@ public: bitmap_data_.set(0xff000000); } - marker_rgba8(image_rgba8 const & data) + explicit marker_rgba8(image_rgba8 const& data) : bitmap_data_(data) {} - marker_rgba8(image_rgba8 && data) + explicit marker_rgba8(image_rgba8 && data) noexcept : bitmap_data_(std::move(data)) {} - marker_rgba8(marker_rgba8 const& rhs) - : bitmap_data_(rhs.bitmap_data_) {} - - marker_rgba8(marker_rgba8 && rhs) noexcept - : bitmap_data_(std::move(rhs.bitmap_data_)) {} - box2d bounding_box() const { std::size_t _width = bitmap_data_.width(); @@ -99,17 +93,11 @@ private: struct marker_svg { public: - marker_svg() { } + marker_svg() = default; - marker_svg(mapnik::svg_path_ptr data) + explicit marker_svg(mapnik::svg_path_ptr data) noexcept : vector_data_(data) {} - marker_svg(marker_svg const& rhs) - : vector_data_(rhs.vector_data_) {} - - marker_svg(marker_svg && rhs) noexcept - : vector_data_(rhs.vector_data_) {} - inline box2d bounding_box() const { return vector_data_->bounding_box(); @@ -140,7 +128,6 @@ private: struct marker_null { - marker_null() = default; public: inline box2d bounding_box() const { diff --git a/include/mapnik/vertex.hpp b/include/mapnik/vertex.hpp index b05f362a5..2e1d37844 100644 --- a/include/mapnik/vertex.hpp +++ b/include/mapnik/vertex.hpp @@ -61,29 +61,12 @@ struct vertex vertex(coord_type x_,coord_type y_,unsigned cmd_) : x(x_),y(y_),cmd(cmd_) {} - vertex(vertex && rhs) noexcept - : x(std::move(rhs.x)), - y(std::move(rhs.y)), - cmd(std::move(rhs.cmd)) {} - - vertex(vertex const& rhs) - : x(rhs.x), - y(rhs.y), - cmd(rhs.cmd) {} - template vertex(vertex const& rhs) : x(coord_type(rhs.x)), y(coord_type(rhs.y)), cmd(rhs.cmd) {} - - vertex& operator=(vertex rhs) - { - swap(rhs); - return *this; - } - template vertex& operator=(vertex const& rhs) { From 400e05585fc32a24138f7428d47b455d62a2d301 Mon Sep 17 00:00:00 2001 From: Mickey Rose Date: Fri, 29 Jan 2016 21:38:40 +0100 Subject: [PATCH 2/4] refine noexcept specifiers on forwarding conversion constructors --- include/mapnik/image_any.hpp | 5 +++-- include/mapnik/image_view_any.hpp | 5 +++-- include/mapnik/marker.hpp | 5 +++-- include/mapnik/params.hpp | 3 ++- include/mapnik/symbolizer_base.hpp | 18 +++++++++--------- 5 files changed, 20 insertions(+), 16 deletions(-) diff --git a/include/mapnik/image_any.hpp b/include/mapnik/image_any.hpp index 735ad8223..8aedf302a 100644 --- a/include/mapnik/image_any.hpp +++ b/include/mapnik/image_any.hpp @@ -55,8 +55,9 @@ struct MAPNIK_DECL image_any : image_base bool painted = false); template - image_any(T && _data) noexcept - : image_base(std::move(_data)) {} + image_any(T && _data) + noexcept(std::is_nothrow_constructible::value) + : image_base(std::forward(_data)) {} unsigned char const* bytes() const; unsigned char* bytes(); diff --git a/include/mapnik/image_view_any.hpp b/include/mapnik/image_view_any.hpp index f61e33db5..23c9779d8 100644 --- a/include/mapnik/image_view_any.hpp +++ b/include/mapnik/image_view_any.hpp @@ -47,8 +47,9 @@ struct MAPNIK_DECL image_view_any : image_view_base image_view_any() = default; template - image_view_any(T && data) noexcept - : image_view_base(std::move(data)) {} + image_view_any(T && data) + noexcept(std::is_nothrow_constructible::value) + : image_view_base(std::forward(data)) {} std::size_t width() const; std::size_t height() const; diff --git a/include/mapnik/marker.hpp b/include/mapnik/marker.hpp index ff7ac7028..8ba117ec3 100644 --- a/include/mapnik/marker.hpp +++ b/include/mapnik/marker.hpp @@ -182,8 +182,9 @@ struct marker : marker_base marker() = default; template - marker(T && _data) noexcept - : marker_base(std::move(_data)) {} + marker(T && _data) + noexcept(std::is_nothrow_constructible::value) + : marker_base(std::forward(_data)) {} double width() const { diff --git a/include/mapnik/params.hpp b/include/mapnik/params.hpp index fc5928900..a154cf731 100644 --- a/include/mapnik/params.hpp +++ b/include/mapnik/params.hpp @@ -50,7 +50,8 @@ struct value_holder : value_holder_base // perfect forwarding template - value_holder(T && obj) noexcept + value_holder(T && obj) + noexcept(std::is_nothrow_constructible::value) : value_holder_base(std::forward(obj)) {} }; diff --git a/include/mapnik/symbolizer_base.hpp b/include/mapnik/symbolizer_base.hpp index f960752c8..8ac446265 100644 --- a/include/mapnik/symbolizer_base.hpp +++ b/include/mapnik/symbolizer_base.hpp @@ -98,10 +98,8 @@ using value_base_type = util::variant::type(obj)) {} - // move ctor - template - strict_value(T && obj) noexcept - : value_base_type(std::move(obj)) {} + template + strict_value(T && obj) + noexcept(std::is_nothrow_constructible::value) + : value_base_type(std::forward(obj)) + {} }; -} + +} // namespace detail struct MAPNIK_DECL symbolizer_base { From be5d772d6c2744059a0a47dd2dc98fa174fb173a Mon Sep 17 00:00:00 2001 From: Mickey Rose Date: Sat, 30 Jan 2016 00:50:22 +0100 Subject: [PATCH 3/4] attempt to fortify mapnik::value conversion construction - including correct noexcept specifier - adding conversion-assignment operator wasn't really necessary, but it might be more efficient since it avoids constructing an intermediate variant --- include/mapnik/value.hpp | 37 +++++++++++++++++++++++++--------- include/mapnik/value_types.hpp | 7 +++++++ 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/include/mapnik/value.hpp b/include/mapnik/value.hpp index 45e2e3136..fae3977d8 100644 --- a/include/mapnik/value.hpp +++ b/include/mapnik/value.hpp @@ -754,17 +754,36 @@ class value : public value_base friend const value operator%(value const&,value const&); public: - value () noexcept //-- comment out for VC++11 - : value_base(value_null()) {} + value() = default; - template - value ( T const& val) - : value_base(typename detail::mapnik_value_type::type(val)) {} + // conversion from type T is done via a temporary of type U, which + // is determined by mapnik_value_type; + // enable_if< decay != value > is necessary to avoid ill-formed + // recursion in noexcept specifier; and it also prevents using this + // constructor where implicitly-declared copy/move should be used + // (e.g. value(value&)) + template ::value, + detail::mapnik_value_type_decay + >::type::type> + value(T && val) + noexcept(noexcept(U(std::forward(val))) && + std::is_nothrow_constructible::value) + : value_base(U(std::forward(val))) {} - template - value ( T && val) - noexcept(std::is_nothrow_move_constructible::type>::value) - : value_base(std::move(typename detail::mapnik_value_type::type(val))) {} + template ::value, + detail::mapnik_value_type_decay + >::type::type> + value& operator=(T && val) + noexcept(noexcept(U(std::forward(val))) && + std::is_nothrow_assignable::value) + { + value_base::operator=(U(std::forward(val))); + return *this; + } bool operator==(value const& other) const { diff --git a/include/mapnik/value_types.hpp b/include/mapnik/value_types.hpp index 83ea7641a..4d508fc79 100644 --- a/include/mapnik/value_types.hpp +++ b/include/mapnik/value_types.hpp @@ -227,6 +227,13 @@ struct mapnik_value_type +using mapnik_value_type_decay = mapnik_value_type::type>; + +template +using is_same_decay = std::is_same::type, + typename std::decay::type>; + } // namespace detail } // namespace mapnik From a8d8a0d74f0bf6b27c2cd30f823624fa1ddca68d Mon Sep 17 00:00:00 2001 From: Mickey Rose Date: Sat, 30 Jan 2016 02:00:28 +0100 Subject: [PATCH 4/4] make clang happy - const default initialization http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#253 http://stackoverflow.com/questions/7411515/why-does-c-require-a-user-provided-default-constructor-to-default-construct-a http://stackoverflow.com/questions/21900237/do-i-really-need-to-implement-user-provided-constructor-for-const-objects --- include/mapnik/feature.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mapnik/feature.hpp b/include/mapnik/feature.hpp index c232075ad..f340c83d6 100644 --- a/include/mapnik/feature.hpp +++ b/include/mapnik/feature.hpp @@ -90,7 +90,7 @@ private: using context_type = context >; using context_ptr = std::shared_ptr; -static const value default_feature_value; +static const value default_feature_value{}; class MAPNIK_DECL feature_impl : private util::noncopyable {