1
0
Fork 0

make the comparison functions unambiguous (#9349)

* make the comparison functions unambiguous

* added @kaveh's suggestion
This commit is contained in:
Jan 2019-07-01 16:35:28 +02:00 committed by GitHub
parent fe19b8aaae
commit 9cb08ded92
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
30 changed files with 120 additions and 98 deletions

View File

@ -224,7 +224,7 @@ class Collection {
struct IsEqualPredicate {
IsEqualPredicate(Slice const& value) : value(value) {}
bool operator()(Slice const& current, ValueLength) {
return value.equals(current);
return value.binaryEquals(current);
}
// compare value
Slice const value;

View File

@ -50,7 +50,7 @@ class ArrayIterator {
throw Exception(Exception::InvalidValueType, "Expecting Array slice");
}
_size = slice.arrayLength(head);
_size = slice.arrayLength();
if (_size > 0) {
VELOCYPACK_ASSERT(head != 0x01); // no empty array allowed here
@ -191,7 +191,7 @@ class ObjectIterator {
throw Exception(Exception::InvalidValueType, "Expecting Object slice");
}
_size = slice.objectLength(head);
_size = slice.objectLength();
if (_size > 0) {
VELOCYPACK_ASSERT(head != 0x0a); // no empty object allowed here

View File

@ -914,7 +914,16 @@ class Slice {
}
// check if two Slices are equal on the binary level
bool equals(Slice const& other) const {
// please note that for several values there are multiple possible representations,
// which differ on the binary level but will still resolve to the same logical
// values. For example, smallint(1) and int(1) are logically the same, but will
// resolve to either 0x31 or 0x28 0x01.
bool binaryEquals(Slice const& other) const {
if (start() == other.start()) {
// same underlying data, so the slices must be identical
return true;
}
if (head() != other.head()) {
return false;
}
@ -927,13 +936,19 @@ class Slice {
return (memcmp(start(), other.start(), checkOverflow(size)) == 0);
}
bool operator==(Slice const& other) const { return equals(other); }
bool operator!=(Slice const& other) const { return !equals(other); }
static bool equals(uint8_t const* left, uint8_t const* right) {
return Slice(left).equals(Slice(right));
static bool binaryEquals(uint8_t const* left, uint8_t const* right) {
return Slice(left).binaryEquals(Slice(right));
}
// these operators are now deleted because they didn't do what people expected
// these operators checked for _binary_ equality of the velocypack slice with
// another. however, for several values there are multiple possible representations,
// which differ on the binary level but will still resolve to the same logical
// values. For example, smallint(1) and int(1) are logically the same, but will
// resolve to either 0x31 or 0x28 0x01.
bool operator==(Slice const& other) const = delete;
bool operator!=(Slice const& other) const = delete;
std::string toHex() const;
std::string toJson(Options const* options = &Options::Defaults) const;
@ -953,27 +968,28 @@ class Slice {
private:
// return the number of members for an Array
// must only be called for Slices that have been validated to be of type Array
ValueLength arrayLength(uint8_t head) const {
VELOCYPACK_ASSERT(type(head) == ValueType::Array);
ValueLength arrayLength() const {
auto const h = head();
VELOCYPACK_ASSERT(type(h) == ValueType::Array);
if (head == 0x01) {
if (h == 0x01) {
// special case: empty!
return 0;
}
if (head == 0x13) {
if (h == 0x13) {
// compact Array
ValueLength end = readVariableValueLength<false>(_start + 1);
return readVariableValueLength<true>(_start + end - 1);
}
ValueLength const offsetSize = indexEntrySize(head);
ValueLength const offsetSize = indexEntrySize(h);
VELOCYPACK_ASSERT(offsetSize > 0);
// find number of items
if (head <= 0x05) { // No offset table or length, need to compute:
VELOCYPACK_ASSERT(head != 0x00 && head != 0x01);
ValueLength firstSubOffset = findDataOffset(head);
if (h <= 0x05) { // No offset table or length, need to compute:
VELOCYPACK_ASSERT(h != 0x00 && h != 0x01);
ValueLength firstSubOffset = findDataOffset(h);
Slice first(_start + firstSubOffset);
ValueLength s = first.byteSize();
if (VELOCYPACK_UNLIKELY(s == 0)) {
@ -991,21 +1007,22 @@ class Slice {
// return the number of members for an Object
// must only be called for Slices that have been validated to be of type Object
ValueLength objectLength(uint8_t head) const {
VELOCYPACK_ASSERT(type(head) == ValueType::Object);
ValueLength objectLength() const {
auto const h = head();
VELOCYPACK_ASSERT(type(h) == ValueType::Object);
if (head == 0x0a) {
if (h == 0x0a) {
// special case: empty!
return 0;
}
if (head == 0x14) {
if (h == 0x14) {
// compact Object
ValueLength end = readVariableValueLength<false>(_start + 1);
return readVariableValueLength<true>(_start + end - 1);
}
ValueLength const offsetSize = indexEntrySize(head);
ValueLength const offsetSize = indexEntrySize(h);
VELOCYPACK_ASSERT(offsetSize > 0);
if (offsetSize < 8) {

View File

@ -128,7 +128,7 @@ bool Collection::contains(Slice const& slice, Slice const& other) {
ArrayIterator it(slice);
while (it.valid()) {
if (it.value().equals(other)) {
if (it.value().binaryEquals(other)) {
return true;
}
it.next();
@ -142,7 +142,7 @@ ValueLength Collection::indexOf(Slice const& slice, Slice const& other) {
ValueLength index = 0;
while (it.valid()) {
if (it.value().equals(other)) {
if (it.value().binaryEquals(other)) {
return index;
}
it.next();

View File

@ -1854,14 +1854,14 @@ bool AgencyComm::shouldInitializeStructure() {
// Sanity
if (result.slice().isArray() && result.slice().length() == 1) {
// No plan entry? Should initialise
if (result.slice()[0] == VPackSlice::emptyObjectSlice()) {
// No plan entry? Should initialize
if (result.slice()[0].isObject() && result.slice()[0].length() == 0) {
LOG_TOPIC("98732", DEBUG, Logger::AGENCYCOMM)
<< "agency initialisation should be performed";
<< "agency initialization should be performed";
return true;
} else {
LOG_TOPIC("abedb", DEBUG, Logger::AGENCYCOMM)
<< "agency initialisation under way or done";
<< "agency initialization under way or done";
return false;
}
} else {

View File

@ -431,7 +431,7 @@ JOB_STATUS FailedLeader::status() {
auto cur_slice = _snapshot.hasAsSlice(curColPrefix + sub + "/" +
clone.shard + "/servers");
if (plan_slice.second && cur_slice.second &&
plan_slice.first[0] != cur_slice.first[0]) {
basics::VelocyPackHelper::compare(plan_slice.first[0], cur_slice.first[0], false) != 0) {
LOG_TOPIC("0d8ca", DEBUG, Logger::SUPERVISION)
<< "FailedLeader waiting for " << sub + "/" + shard;
break;

View File

@ -150,12 +150,10 @@ bool Job::finish(std::string const& server, std::string const& shard,
} // -- operations
if (preconditions != Slice::emptyObjectSlice()) { // preconditions --
if (preconditions.isObject() && preconditions.length() > 0) { // preconditions --
VPackObjectBuilder precguard(&finished);
if (preconditions.length() > 0) {
for (auto const& prec : VPackObjectIterator(preconditions)) {
finished.add(prec.key.copyString(), prec.value);
}
for (auto const& prec : VPackObjectIterator(preconditions)) {
finished.add(prec.key.copyString(), prec.value);
}
} // -- preconditions
@ -489,9 +487,9 @@ std::string Job::findNonblockedCommonHealthyInSyncFollower( // Which is in "GOO
bool found = false;
for (const auto& plannedServer :
VPackArrayIterator(snap.hasAsArray(plannedShardPath).first)) {
if (plannedServer == server) {
if (plannedServer.isEqualString(server.stringRef())) {
found = true;
continue;
break;
}
}

View File

@ -444,18 +444,17 @@ check_ret_t Store::check(VPackSlice const& slice, CheckMode mode) const {
} else if (oper == "in") { // in
if (found) {
if (node->slice().isArray()) {
bool _found = false;
bool found = false;
for (auto const& i : VPackArrayIterator(node->slice())) {
if (i == op.value) {
_found = true;
continue;
if (basics::VelocyPackHelper::compare(i, op.value, false) == 0) {
found = true;
break;
}
}
if (_found) {
if (found) {
continue;
} else {
ret.push_back(precond.key);
}
}
ret.push_back(precond.key);
}
}
ret.push_back(precond.key);
@ -465,21 +464,19 @@ check_ret_t Store::check(VPackSlice const& slice, CheckMode mode) const {
} else if (oper == "notin") { // in
if (!found) {
continue;
} else {
if (node->slice().isArray()) {
bool _found = false;
for (auto const& i : VPackArrayIterator(node->slice())) {
if (i == op.value) {
_found = true;
continue;
}
}
if (_found) {
ret.push_back(precond.key);
} else {
continue;
}
if (node->slice().isArray()) {
bool found = false;
for (auto const& i : VPackArrayIterator(node->slice())) {
if (basics::VelocyPackHelper::compare(i, op.value, false) == 0) {
found = true;
break;
}
}
if (!found) {
continue;
}
ret.push_back(precond.key);
}
ret.push_back(precond.key);
if (mode == FIRST_FAIL) {

View File

@ -1318,7 +1318,7 @@ void Supervision::readyOrphanedIndexCreations() {
currentDBs(colPath + shname + "/indexes").slice();
for (auto const& curIndex : VPackArrayIterator(curIndexes)) {
auto const& curId = curIndex.get("id");
if (planId == curId) {
if (basics::VelocyPackHelper::compare(planId, curId, false) == 0) {
++nIndexes;
}
}

View File

@ -770,7 +770,7 @@ struct equal_to<arangodb::aql::AqlValue> {
if (type == arangodb::aql::AqlValue::VPACK_INLINE) {
try {
return arangodb::velocypack::Slice(&a._data.internal[0])
.equals(arangodb::velocypack::Slice(&b._data.internal[0]));
.binaryEquals(arangodb::velocypack::Slice(&b._data.internal[0]));
} catch (...) {
TRI_ASSERT(false);
}

View File

@ -5961,7 +5961,8 @@ AqlValue Functions::Append(ExpressionContext* expressionContext, transaction::Me
return AqlValue(AqlValueHintNull());
}
std::unordered_set<VPackSlice> added;
std::unordered_set<VPackSlice, basics::VelocyPackHelper::VPackHash, basics::VelocyPackHelper::VPackEqual> added(
11, basics::VelocyPackHelper::VPackHash(), basics::VelocyPackHelper::VPackEqual());
transaction::BuilderLeaser builder(trx);
builder->openArray();

View File

@ -1490,7 +1490,7 @@ class PropagateConstantAttributesHelper {
return;
}
if (!value->computeValue().equals(previous->computeValue())) {
if (!value->computeValue().binaryEquals(previous->computeValue())) {
// different value found for an already tracked attribute. better not
// use this attribute
(*it2).second = nullptr;

View File

@ -33,6 +33,7 @@
#include "ApplicationFeatures/ApplicationServer.h"
#include "Basics/ConditionLocker.h"
#include "Basics/MutexLocker.h"
#include "Basics/VelocyPackHelper.h"
#include "Logger/Logger.h"
using namespace arangodb;
@ -90,7 +91,7 @@ void AgencyCallback::refetchAndUpdate(bool needToAcquireMutex, bool forceCheck)
void AgencyCallback::checkValue(std::shared_ptr<VPackBuilder> newData, bool forceCheck) {
// Only called from refetchAndUpdate, we always have the mutex when
// we get here!
if (!_lastData || !_lastData->slice().equals(newData->slice()) || forceCheck) {
if (!_lastData || arangodb::basics::VelocyPackHelper::compare(_lastData->slice(), newData->slice(), false) != 0 || forceCheck) {
LOG_TOPIC("2bd14", DEBUG, Logger::CLUSTER)
<< "AgencyCallback: Got new value " << newData->slice().typeName()
<< " " << newData->toJson() << " forceCheck=" << forceCheck;

View File

@ -3647,7 +3647,7 @@ void ClusterInfo::loadCurrentDBServers() {
bool found = false;
if (failedDBServers.isObject()) {
for (auto const& failedServer : VPackObjectIterator(failedDBServers)) {
if (dbserver.key == failedServer.key) {
if (basics::VelocyPackHelper::compare(dbserver.key, failedServer.key, false) == 0) {
found = true;
break;
}
@ -3660,7 +3660,7 @@ void ClusterInfo::loadCurrentDBServers() {
if (cleanedDBServers.isArray()) {
bool found = false;
for (auto const& cleanedServer : VPackArrayIterator(cleanedDBServers)) {
if (dbserver.key == cleanedServer) {
if (basics::VelocyPackHelper::compare(dbserver.key, cleanedServer, false) == 0) {
found = true;
break;
}
@ -3673,7 +3673,7 @@ void ClusterInfo::loadCurrentDBServers() {
if (toBeCleanedDBServers.isArray()) {
bool found = false;
for (auto const& toBeCleanedServer : VPackArrayIterator(toBeCleanedDBServers)) {
if (dbserver.key == toBeCleanedServer) {
if (basics::VelocyPackHelper::compare(dbserver.key, toBeCleanedServer, false) == 0) {
found = true;
break;
}

View File

@ -314,7 +314,7 @@ static void mergeResultsAllShards(std::vector<std::shared_ptr<VPackBuilder>> con
VPackSlice oneRes = it->slice();
TRI_ASSERT(oneRes.isArray());
oneRes = oneRes.at(currentIndex);
if (!oneRes.equals(notFound)) {
if (basics::VelocyPackHelper::compare(oneRes, notFound, false) != 0) {
// This is the correct result
// Use it
resultBody->add(oneRes);

View File

@ -85,7 +85,7 @@ static std::shared_ptr<VPackBuilder> compareRelevantProps(VPackSlice const& firs
VPackObjectBuilder b(result.get());
for (auto const& property : cmp) {
auto const& planned = first.get(property);
if (planned != second.get(property)) { // Register any change
if (basics::VelocyPackHelper::compare(planned, second.get(property), false) != 0) { // Register any change
result->add(property, planned);
}
}
@ -233,7 +233,7 @@ void handlePlanShard(VPackSlice const& cprops, VPackSlice const& ldb,
}
// If comparison has brought any updates
if (properties->slice() != VPackSlice::emptyObjectSlice() ||
if (!properties->slice().isObject() || properties->slice().length() > 0 ||
leading != shouldBeLeading || !followersToDropString.empty()) {
if (errors.shards.find(fullShardLabel) == errors.shards.end()) {
actions.emplace_back(ActionDescription(

View File

@ -118,8 +118,7 @@ struct EdgeDocumentToken {
#ifdef ARANGODB_ENABLE_MAINTAINER_MODE
TRI_ASSERT(_type == TokenType::COORDINATOR);
#endif
// FIXME:
return velocypack::Slice(_data.vpack) == velocypack::Slice(other._data.vpack);
return velocypack::Slice(_data.vpack).binaryEquals(velocypack::Slice(other._data.vpack));
}
bool equalsLocal(EdgeDocumentToken const& other) const {

View File

@ -88,6 +88,7 @@ class KShortestPathsFinder : public ShortestPathFinder {
// Only append paths where the first vertex of p
// is the same as the last vertex of this.
TRI_ASSERT((_vertices.back().equals(p._vertices.front())));
TRI_ASSERT(!_weights.empty());
double ew = _weights.back();
double pw = p._weights.at(a);
@ -98,7 +99,7 @@ class KShortestPathsFinder : public ShortestPathFinder {
_weights.emplace_back(ew + (p._weights.at(a) - pw));
}
_weight = _weights.back();
};
}
// TODO: implement == for EdgeDocumentToken and VertexRef
// so these things become less cluttery
bool operator==(Path const& rhs) const {
@ -107,12 +108,12 @@ class KShortestPathsFinder : public ShortestPathFinder {
return false;
}
for (size_t i = 0; i < _vertices.size(); ++i) {
if (!_vertices.at(i).equals(rhs._vertices.at(i))) {
if (!_vertices[i].equals(rhs._vertices[i])) {
return false;
}
}
for (size_t i = 0; i < _edges.size(); ++i) {
if (!_edges.at(i).equals(rhs._edges.at(i))) {
if (!_edges[i].equals(rhs._edges[i])) {
return false;
}
}

View File

@ -106,15 +106,18 @@ bool DepthFirstEnumerator::next() {
}
if (_opts->uniqueEdges == TraverserOptions::UniquenessLevel::PATH) {
ServerState::RoleEnum role = ServerState::instance()->getRole();
for (auto const& it : _enumeratedPath.edges) {
// We might already have this edge on the path.
if (ServerState::isCoordinator(role)) {
if (ServerState::instance()->isCoordinator()) {
for (auto const& it : _enumeratedPath.edges) {
// We might already have this edge on the path.
if (it.equalsCoordinator(eid)) {
return;
}
} else if (it.equalsLocal(eid)) {
return;
}
} else {
for (auto const& it : _enumeratedPath.edges) {
if (it.equalsLocal(eid)) {
return;
}
}
}
}

View File

@ -577,7 +577,7 @@ namespace iresearch {
auto lhsViewSlice = lhs.get(StaticStrings::ViewIdField);
auto rhsViewSlice = rhs.get(StaticStrings::ViewIdField);
if (!lhsViewSlice.equals(rhsViewSlice)) {
if (!lhsViewSlice.binaryEquals(rhsViewSlice)) {
if (!lhsViewSlice.isString() || !rhsViewSlice.isString()) {
return false;
}

View File

@ -25,6 +25,7 @@
#include "utils/locale_utils.hpp"
#include "Basics/StringUtils.h"
#include "Basics/VelocyPackHelper.h"
#include "IResearchCommon.h"
#include "VelocyPackHelper.h"
#include "VocBase/LogicalView.h"
@ -285,7 +286,7 @@ bool IResearchViewMeta::operator==(IResearchViewMeta const& other) const noexcep
}
try {
if (!_consolidationPolicy.properties().equals(other._consolidationPolicy.properties())) {
if (basics::VelocyPackHelper::compare(_consolidationPolicy.properties(), other._consolidationPolicy.properties(), false) != 0) {
return false; // values do not match
}
} catch (...) {
@ -620,8 +621,9 @@ bool IResearchViewMeta::json(arangodb::velocypack::Builder& builder,
arangodb::velocypack::Value(_consolidationIntervalMsec));
}
if ((!ignoreEqual || !_consolidationPolicy.properties().equals(
ignoreEqual->_consolidationPolicy.properties())) &&
if ((!ignoreEqual || arangodb::basics::VelocyPackHelper::compare(
_consolidationPolicy.properties(),
ignoreEqual->_consolidationPolicy.properties(), false) != 0) &&
(!mask || mask->_consolidationPolicy)) {
builder.add("consolidationPolicy", _consolidationPolicy.properties());
}

View File

@ -74,7 +74,7 @@ struct MMFilesEdgeIndexHelper {
try {
VPackSlice tmp = right.slice(context);
TRI_ASSERT(tmp.isString());
return left->equals(tmp);
return left->binaryEquals(tmp);
} catch (...) {
return false;
}
@ -98,7 +98,7 @@ struct MMFilesEdgeIndexHelper {
TRI_ASSERT(lSlice.isString());
TRI_ASSERT(rSlice.isString());
return lSlice.equals(rSlice);
return lSlice.binaryEquals(rSlice);
} catch (...) {
return false;
}

View File

@ -63,7 +63,7 @@ struct MMFilesPrimaryIndexHelper {
try {
VPackSlice tmp = right.slice(context);
TRI_ASSERT(tmp.isString());
return VPackSlice(key).equals(tmp);
return VPackSlice(key).binaryEquals(tmp);
} catch (...) {
return false;
}
@ -89,7 +89,7 @@ struct MMFilesPrimaryIndexHelper {
VPackSlice r = right.slice(context);
TRI_ASSERT(l.isString());
TRI_ASSERT(r.isString());
return l.equals(r);
return l.binaryEquals(r);
}
};

View File

@ -328,7 +328,7 @@ Result TailingSyncer::processDBMarker(TRI_replication_operation_e type,
TRI_ERROR_REPLICATION_INVALID_RESPONSE,
"create database marker did not contain data");
}
TRI_ASSERT(data.get("name") == nameSlice);
TRI_ASSERT(basics::VelocyPackHelper::compare(data.get("name"), nameSlice, false) == 0);
TRI_vocbase_t* vocbase = DatabaseFeature::DATABASE->lookupDatabase(name);

View File

@ -250,7 +250,7 @@ void runActiveFailoverStart(std::string const& myId) {
if (leader.isString() && leader.getStringLength() > 0) {
ServerState::instance()->setFoxxmaster(leader.copyString());
if (leader == myIdBuilder.slice()) {
if (basics::VelocyPackHelper::compare(leader, myIdBuilder.slice(), false) == 0) {
LOG_TOPIC("95023", INFO, Logger::STARTUP)
<< "Became leader in active-failover setup";
} else {

View File

@ -625,7 +625,7 @@ Result RocksDBPrimaryIndex::update(transaction::Methods& trx, RocksDBMethods* mt
Index::OperationMode mode) {
Result res;
VPackSlice keySlice = transaction::helpers::extractKeyFromDocument(oldDoc);
TRI_ASSERT(keySlice == oldDoc.get(StaticStrings::KeyString));
TRI_ASSERT(keySlice.binaryEquals(oldDoc.get(StaticStrings::KeyString)));
RocksDBKeyLeaser key(&trx);
key->constructPrimaryIndexValue(_objectId, arangodb::velocypack::StringRef(keySlice));

View File

@ -397,7 +397,7 @@ template <>
struct equal_to<arangodb::basics::VPackHashedSlice> {
bool operator()(arangodb::basics::VPackHashedSlice const& lhs,
arangodb::basics::VPackHashedSlice const& rhs) const {
return lhs.slice.equals(rhs.slice);
return lhs.slice.binaryEquals(rhs.slice);
}
};

View File

@ -23,6 +23,7 @@
#include "AqlHelper.h"
#include "Aql/ExecutionStats.h"
#include "Basics/VelocyPackHelper.h"
using namespace arangodb;
using namespace arangodb::aql;
@ -90,7 +91,7 @@ bool arangodb::aql::operator==(arangodb::aql::AqlItemBlock const& left,
AqlValue const& l = left.getValueReference(row, reg);
AqlValue const& r = right.getValueReference(row, reg);
// Doesn't work for docvecs or ranges
if (l.slice() != r.slice()) {
if (arangodb::basics::VelocyPackHelper::compare(l.slice(), r.slice(), false) != 0) {
return false;
}
}

View File

@ -54,8 +54,10 @@ namespace aql {
class TokenTranslator : public TraverserCache {
public:
TokenTranslator(Query* query) : TraverserCache(query) {}
~TokenTranslator(){};
TokenTranslator(Query* query)
: TraverserCache(query),
_edges(11, arangodb::basics::VelocyPackHelper::VPackHash(), arangodb::basics::VelocyPackHelper::VPackEqual()) {}
~TokenTranslator() {}
arangodb::velocypack::StringRef makeVertex(std::string const& id) {
VPackBuilder vertex;
@ -105,7 +107,7 @@ class TokenTranslator : public TraverserCache {
private:
std::vector<std::shared_ptr<VPackBuffer<uint8_t>>> _dataLake;
std::unordered_map<arangodb::velocypack::StringRef, VPackSlice> _vertices;
std::unordered_set<VPackSlice> _edges;
std::unordered_set<VPackSlice, arangodb::basics::VelocyPackHelper::VPackHash, arangodb::basics::VelocyPackHelper::VPackEqual> _edges;
};
class FakePathFinder : public ShortestPathFinder {

View File

@ -832,7 +832,7 @@ arangodb::Result PhysicalCollectionMock::remove(
arangodb::velocypack::Builder& doc = entry.first;
if (key == doc.slice().get(arangodb::StaticStrings::KeyString)) {
if (arangodb::basics::VelocyPackHelper::compare(key, doc.slice().get(arangodb::StaticStrings::KeyString), false) == 0) {
entry.second = false;
previous.setUnmanaged(doc.data());
TRI_ASSERT(previous.revisionId() == TRI_ExtractRevisionId(doc.slice()));
@ -896,7 +896,7 @@ arangodb::Result PhysicalCollectionMock::update(
auto& doc = entry.first;
if (key == doc.slice().get(arangodb::StaticStrings::KeyString)) {
if (arangodb::basics::VelocyPackHelper::compare(key, doc.slice().get(arangodb::StaticStrings::KeyString), false) == 0) {
if (!options.mergeObjects) {
entry.second = false;
previous.setUnmanaged(doc.data());