bahman/scylla
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

Use std::reference_wrapper instead of a plain reference in bound_view.

Browse Source 
The presence of a plain reference prohibits the bound_view class from
being copyable. The trick employed to work around that was to use
'placement new' for copy-assigning bound_view objects, but this approach
is ill-formed and causes undefined behaviour for classes that have const
and/or reference members.

The solution is to use a std::reference_wrapper instead.

Signed-off-by: Vladimir Krivopalov <vladimir@scylladb.com>
Message-Id: <a0c951649c7aef2f66612fc006c44f8a33713931.1530113273.git.vladimir@scylladb.com>
This commit is contained in:
Vladimir Krivopalov
2018-06-27 08:53:00 -07:00
committed by Paweł Dziepak
parent c87a961667
commit 82f76b0947
8 changed files with 40 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
clustering_bounds_comparator.hh
View File

@@ -22,6 +22,7 @@
#pragma once
#include <functional>
#include "keys.hh"
#include "schema.hh"
#include "range.hh"
@@ -43,22 +44,20 @@ bound_kind invert_kind(bound_kind k);
int32_t weight(bound_kind k);
class bound_view {
const static thread_local clustering_key _empty_prefix;
std::reference_wrapper<const clustering_key_prefix> _prefix;
bound_kind _kind;
public:
const static thread_local clustering_key empty_prefix;
const clustering_key_prefix& prefix;
bound_kind kind;
bound_view(const clustering_key_prefix& prefix, bound_kind kind)
: prefix(prefix)
, kind(kind)
: _prefix(prefix)
, _kind(kind)
{ }
bound_view(const bound_view& other) noexcept = default;
bound_view& operator=(const bound_view& other) noexcept {
if (this != &other) {
this->~bound_view();
new (this) bound_view(other);
}
return *this;
}
bound_view& operator=(const bound_view& other) noexcept = default;
bound_kind kind() const { return _kind; }
const clustering_key_prefix& prefix() const { return _prefix; }
struct tri_compare {
// To make it assignable and to avoid taking a schema_ptr, we
// wrap the schema reference.
@@ -82,13 +81,13 @@ public:
return d1 < d2 ? w1 - (w1 <= 0) : -(w2 - (w2 <= 0));
}
int operator()(const bound_view b, const clustering_key_prefix& p) const {
return operator()(b.prefix, weight(b.kind), p, 0);
return operator()(b._prefix, weight(b._kind), p, 0);
}
int operator()(const clustering_key_prefix& p, const bound_view b) const {
return operator()(p, 0, b.prefix, weight(b.kind));
return operator()(p, 0, b._prefix, weight(b._kind));
}
int operator()(const bound_view b1, const bound_view b2) const {
return operator()(b1.prefix, weight(b1.kind), b2.prefix, weight(b2.kind));
return operator()(b1._prefix, weight(b1._kind), b2._prefix, weight(b2._kind));
}
};
struct compare {
@@ -101,26 +100,26 @@ public:
return _cmp(p1, w1, p2, w2) < 0;
}
bool operator()(const bound_view b, const clustering_key_prefix& p) const {
return operator()(b.prefix, weight(b.kind), p, 0);
return operator()(b._prefix, weight(b._kind), p, 0);
}
bool operator()(const clustering_key_prefix& p, const bound_view b) const {
return operator()(p, 0, b.prefix, weight(b.kind));
return operator()(p, 0, b._prefix, weight(b._kind));
}
bool operator()(const bound_view b1, const bound_view b2) const {
return operator()(b1.prefix, weight(b1.kind), b2.prefix, weight(b2.kind));
return operator()(b1._prefix, weight(b1._kind), b2._prefix, weight(b2._kind));
}
};
bool equal(const schema& s, const bound_view other) const {
return kind == other.kind && prefix.equal(s, other.prefix);
return _kind == other._kind && _prefix.get().equal(s, other._prefix.get());
}
bool adjacent(const schema& s, const bound_view other) const {
return invert_kind(other.kind) == kind && prefix.equal(s, other.prefix);
return invert_kind(other._kind) == _kind && _prefix.get().equal(s, other._prefix.get());
}
static bound_view bottom() {
return {empty_prefix, bound_kind::incl_start};
return {_empty_prefix, bound_kind::incl_start};
}
static bound_view top() {
return {empty_prefix, bound_kind::incl_end};
return {_empty_prefix, bound_kind::incl_end};
}
template<template<typename> typename R>
GCC6_CONCEPT( requires Range<R, clustering_key_prefix_view> )
@@ -144,13 +143,13 @@ public:
template<template<typename> typename R>
GCC6_CONCEPT( requires Range<R, clustering_key_prefix_view> )
static stdx::optional<typename R<clustering_key_prefix_view>::bound> to_range_bound(const bound_view& bv) {
if (&bv.prefix == &empty_prefix) {
if (&bv._prefix.get() == &_empty_prefix) {
return {};
}
bool inclusive = bv.kind != bound_kind::excl_end && bv.kind != bound_kind::excl_start;
return {typename R<clustering_key_prefix_view>::bound(bv.prefix.view(), inclusive)};
bool inclusive = bv._kind != bound_kind::excl_end && bv._kind != bound_kind::excl_start;
return {typename R<clustering_key_prefix_view>::bound(bv._prefix.get().view(), inclusive)};
}
friend std::ostream& operator<<(std::ostream& out, const bound_view& b) {
return out << "{bound: prefix=" << b.prefix << ", kind=" << b.kind << "}";
return out << "{bound: prefix=" << b._prefix.get() << ", kind=" << b._kind << "}";
}
};

2
keys.cc
View File

@@ -113,4 +113,4 @@ int32_t weight(bound_kind k) {
abort();
}
const thread_local clustering_key_prefix bound_view::empty_prefix = clustering_key::make_empty();
const thread_local clustering_key_prefix bound_view::_empty_prefix = clustering_key::make_empty();

4
position_in_partition.hh
View File

@@ -121,7 +121,7 @@ public:
position_in_partition_view(const clustering_key_prefix& ck)
: _type(partition_region::clustered), _ck(&ck) { }
position_in_partition_view(range_tag_t, bound_view bv)
: _type(partition_region::clustered), _bound_weight(position_weight(bv.kind)), _ck(&bv.prefix) { }
: _type(partition_region::clustered), _bound_weight(position_weight(bv.kind())), _ck(&bv.prefix()) { }
static position_in_partition_view for_range_start(const query::clustering_range& r) {
return {position_in_partition_view::range_tag_t(), bound_view::from_range_start(r)};
@@ -214,7 +214,7 @@ public:
position_in_partition(before_clustering_row_tag_t, clustering_key_prefix ck)
: _type(partition_region::clustered), _bound_weight(-1), _ck(std::move(ck)) { }
position_in_partition(range_tag_t, bound_view bv)
: _type(partition_region::clustered), _bound_weight(position_weight(bv.kind)), _ck(bv.prefix) { }
: _type(partition_region::clustered), _bound_weight(position_weight(bv.kind())), _ck(bv.prefix()) { }
position_in_partition(after_static_row_tag_t) :
position_in_partition(range_tag_t(), bound_view::bottom()) { }
explicit position_in_partition(position_in_partition_view view)

4
range_tombstone.cc
View File

@@ -68,10 +68,10 @@ void range_tombstone_accumulator::drop_unneeded_tombstones(const clustering_key_
auto cmp = [&] (const range_tombstone& rt, const clustering_key_prefix& ck, int w) {
if (_reversed) {
auto bv = rt.start_bound();
return _cmp(ck, w, bv.prefix, weight(bv.kind));
return _cmp(ck, w, bv.prefix(), weight(bv.kind()));
}
auto bv = rt.end_bound();
return _cmp(bv.prefix, weight(bv.kind), ck, w);
return _cmp(bv.prefix(), weight(bv.kind()), ck, w);
};
while (!_range_tombstones.empty() && cmp(*_range_tombstones.begin(), ck, w)) {
_range_tombstones.pop_front();

9
range_tombstone.hh
View File

@@ -52,7 +52,7 @@ public:
, tomb(std::move(tomb))
{ }
range_tombstone(bound_view start, bound_view end, tombstone tomb)
: range_tombstone(start.prefix, start.kind, end.prefix, end.kind, std::move(tomb))
: range_tombstone(start.prefix(), start.kind(), end.prefix(), end.kind(), std::move(tomb))
{ }
range_tombstone(clustering_key_prefix&& start, clustering_key_prefix&& end, tombstone tomb)
: range_tombstone(std::move(start), bound_kind::incl_start, std::move(end), bound_kind::incl_end, std::move(tomb))
@@ -151,6 +151,9 @@ public:
}
if (less(position(), pos)) {
set_start(s, pos);
bound_view new_start = pos.as_start_bound_view();
start = new_start.prefix();
start_kind = new_start.kind();
}
return true;
}
@@ -158,8 +161,8 @@ public:
// Assumes !pos.is_clustering_row(), because range_tombstone bounds can't represent such positions
void set_start(const schema& s, position_in_partition_view pos) {
bound_view new_start = pos.as_start_bound_view();
start = new_start.prefix;
start_kind = new_start.kind;
start = new_start.prefix();
start_kind = new_start.kind();
}
size_t external_memory_usage(const schema&) const {

4
range_tombstone_list.cc
View File

@@ -252,7 +252,7 @@ range_tombstone_list range_tombstone_list::difference(const schema& s, const ran
++other_rt;
continue;
}
auto new_end = bound_view(other_rt->start_bound().prefix, invert_kind(other_rt->start_bound().kind));
auto new_end = bound_view(other_rt->start_bound().prefix(), invert_kind(other_rt->start_bound().kind()));
if (cmp_rt(cur_start, new_end)) {
diff.apply(s, cur_start, new_end, this_rt->tomb);
cur_start = other_rt->start_bound();
@@ -267,7 +267,7 @@ range_tombstone_list range_tombstone_list::difference(const schema& s, const ran
if (this_rt->tomb > other_rt->tomb) {
diff.apply(s, cur_start, end, this_rt->tomb);
}
cur_start = bound_view(end.prefix, invert_kind(end.kind));
cur_start = bound_view(end.prefix(), invert_kind(end.kind()));
++other_rt;
if (cmp_rt(cur_end, cur_start)) {
advance_this_rt();

2
range_tombstone_list.hh
View File

@@ -125,7 +125,7 @@ public:
return _tombstones.end();
}
void apply(const schema& s, const bound_view& start_bound, const bound_view& end_bound, tombstone tomb) {
apply(s, start_bound.prefix, start_bound.kind, end_bound.prefix, end_bound.kind, std::move(tomb));
apply(s, start_bound.prefix(), start_bound.kind(), end_bound.prefix(), end_bound.kind(), std::move(tomb));
}
void apply(const schema& s, const range_tombstone& rt) {
apply(s, rt.start, rt.start_kind, rt.end, rt.end_kind, rt.tomb);

2
service/pager/query_pagers.cc
View File

@@ -166,7 +166,7 @@ static bool has_clustering_keys(const schema& s, const query::read_command& cmd)
auto it = ranges.begin();
while (it != ranges.end()) {
auto range = bound_view::from_range(*it);
if (cmp(end_bound(range), lo) || eq(end_bound(range).prefix, lo)) {
if (cmp(end_bound(range), lo) || eq(end_bound(range).prefix(), lo)) {
qlogger.trace("Remove ck range {}", *it);
it = ranges.erase(it);
continue;