Merge branch 'smart_ptr' of https://github.com/MaxKellermann/zeek

Minor whitespace fixes during merge.

* 'smart_ptr' of https://github.com/MaxKellermann/zeek:
  OpaqueVal: remove misplaced `virtual` keywords
  CompHash: use class IntrusivePtr for the `type` field
  IntrusivePtr: replace the "add_ref" parameter with tag structs
  IntrusivePtr: remove reset(), nobody uses it
  IntrusivePtr: remove ordering operators
  IntrusivePtr: rename detach() to release()
  IntrusivePtr: move nullptr initializer to field declaration

CHANGES

@@ -1,4 +1,39 @@
 
+3.2.0-dev.80 | 2020-02-21 10:14:05 -0800
+
+  * OpaqueVal: remove misplaced `virtual` keywords (Max Kellermann)
+
+    These methods are not meant to be overridden.
+
+  * CompHash: use class IntrusivePtr for the `type` field (Max Kellermann)
+
+  * IntrusivePtr: replace the "add_ref" parameter with tag structs (Max Kellermann)
+
+    Using a runtime parameter is obscure and error-prone.  Avoiding
+    error-prone code and getting reference counting right is the whole
+    point of this class.
+
+  * IntrusivePtr: remove reset(), nobody uses it (Max Kellermann)
+
+    This method mimicks std::unique_ptr::reset(), but adds an obscure
+    "add_ref" parameter which is error prone.  Since nobody uses this
+    method, and this method is all about dealing with raw pointers which
+    we shouldn't be doing, let's remove it.
+
+  * IntrusivePtr: remove ordering operators (Max Kellermann)
+
+    These violate the C++ standard because comparing pointers to unrelated
+    objects is undefined behavior.
+
+  * IntrusivePtr: rename detach() to release() (Max Kellermann)
+
+    Follow the C++ standard library conventions (here: `std::unique_ptr`).
+
+  * IntrusivePtr: move nullptr initializer to field declaration (Max Kellermann)
+
+    This allows "defaulting" the default constructor, and guarantees that
+    all constructors really initialize the field to a legal value.
+
 3.2.0-dev.71 | 2020-02-20 14:57:58 -0800
 
   * Fix missing reference count incrment in AssignExpr::InitVal() (Max Kellermann)

VERSION

@@ -1 +1 @@
-3.2.0-dev.71
+3.2.0-dev.80

src/CompHash.cc

@@ -13,10 +13,9 @@
 #include <vector>
 #include <map>
 
-CompositeHash::CompositeHash(TypeList* composite_type)
+CompositeHash::CompositeHash(IntrusivePtr<TypeList> composite_type)
+	: type(std::move(composite_type))
 	{
-	type = composite_type;
-	Ref(type);
 	singleton_tag = TYPE_INTERNAL_ERROR;
 
 	// If the only element is a record, don't treat it as a
@@ -66,7 +65,6 @@ CompositeHash::CompositeHash(TypeList* composite_type)
 
 CompositeHash::~CompositeHash()
 	{
-	Unref(type);
 	delete [] key;
 	}
 

src/CompHash.h

@@ -3,13 +3,14 @@
 #pragma once
 
 #include "Type.h"
+#include "IntrusivePtr.h"
 
 class ListVal;
 class HashKey;
 
 class CompositeHash {
 public:
-	explicit CompositeHash(TypeList* composite_type);
+	explicit CompositeHash(IntrusivePtr<TypeList> composite_type);
 	~CompositeHash();
 
 	// Compute the hash corresponding to the given index val,
@@ -78,7 +79,7 @@ protected:
 			      int type_check, int sz, bool optional,
 			      bool calc_static_size) const;
 
-	TypeList* type;
+	IntrusivePtr<TypeList> type;
 	char* key;	// space for composite key
 	int size;
 	int is_singleton;	// if just one type in index

src/Expr.cc

@@ -2828,7 +2828,7 @@ Val* IndexExpr::Fold(Val* v1, Val* v2) const
 
 void IndexExpr::Assign(Frame* f, Val* arg_v)
 	{
-	IntrusivePtr v{arg_v, false};
+	IntrusivePtr v{AdoptRef{}, arg_v};
 
 	if ( IsError() )
 		return;
@@ -2849,7 +2849,7 @@ void IndexExpr::Assign(Frame* f, Val* arg_v)
 	// Hold an extra reference to 'arg_v' in case the ownership transfer to
 	// the table/vector goes wrong and we still want to obtain diagnostic info
 	// from the original value after the assignment already unref'd.
-	IntrusivePtr v_extra{arg_v, true};
+	IntrusivePtr v_extra{NewRef{}, arg_v};
 
 	switch ( v1->Type()->Tag() ) {
 	case TYPE_VECTOR:
@@ -2873,7 +2873,7 @@ void IndexExpr::Assign(Frame* f, Val* arg_v)
 			for ( auto idx = 0u; idx < v_vect->Size(); idx++, first++ )
 				v1_vect->Insert(first, v_vect->Lookup(idx)->Ref());
 			}
-		else if ( ! v1_vect->Assign(v2, v.detach()) )
+		else if ( ! v1_vect->Assign(v2, v.release()) )
 			{
 			v = v_extra;
 
@@ -2895,7 +2895,7 @@ void IndexExpr::Assign(Frame* f, Val* arg_v)
 		}
 
 	case TYPE_TABLE:
-		if ( ! v1->AsTableVal()->Assign(v2, v.detach()) )
+		if ( ! v1->AsTableVal()->Assign(v2, v.release()) )
 			{
 			v = v_extra;
 

src/Frame.cc

@@ -485,7 +485,7 @@ std::pair<bool, Frame*> Frame::Unserialize(const broker::vector& data)
 			return std::make_pair(false, nullptr);
 			}
 
-		rf->frame[i] = val.detach();
+		rf->frame[i] = val.release();
 		}
 
 	return std::make_pair(true, rf);

src/IntrusivePtr.h

@@ -5,6 +5,18 @@
 #include <type_traits>
 #include <utility>
 
+/**
+ * A tag class for the #IntrusivePtr constructor which means: adopt
+ * the reference from the caller.
+ */
+struct AdoptRef {};
+
+/**
+ * A tag class for the #IntrusivePtr constructor which means: create a
+ * new reference to the object.
+ */
+struct NewRef {};
+
 /**
  * An intrusive, reference counting smart pointer implementation. Much like
  * @c std::shared_ptr, this smart pointer models shared ownership of an object
@@ -44,10 +56,7 @@ public:
 
 	// -- constructors, destructors, and assignment operators
 
-	constexpr IntrusivePtr() noexcept : ptr_(nullptr)
+	constexpr IntrusivePtr() noexcept = default;
-		{
-		// nop
-		}
 
 	constexpr IntrusivePtr(std::nullptr_t) noexcept : IntrusivePtr()
 		{
@@ -57,27 +66,41 @@ public:
 	/**
 	 * Constructs a new intrusive pointer for managing the lifetime of the object
 	 * pointed to by @c raw_ptr.
+	 *
+	 * This overload adopts the existing reference from the caller.
+	 *
 	 * @param raw_ptr Pointer to the shared object.
-	 * @param add_ref Denotes whether the reference count of the object shall be
-	 *                increased during construction.
 	 */
-	IntrusivePtr(pointer raw_ptr, bool add_ref) noexcept
+	IntrusivePtr(AdoptRef, pointer raw_ptr) noexcept
 		{
-		setPtr(raw_ptr, add_ref);
+		setPtr(raw_ptr, false);
 		}
 
-	IntrusivePtr(IntrusivePtr&& other) noexcept : ptr_(other.detach())
+	/**
+	 * Constructs a new intrusive pointer for managing the lifetime of the object
+	 * pointed to by @c raw_ptr.
+	 *
+	 * This overload adds a new reference.
+	 *
+	 * @param raw_ptr Pointer to the shared object.
+	 */
+	IntrusivePtr(NewRef, pointer raw_ptr) noexcept
+		{
+		setPtr(raw_ptr, true);
+		}
+
+	IntrusivePtr(IntrusivePtr&& other) noexcept : ptr_(other.release())
 		{
 		// nop
 		}
 
 	IntrusivePtr(const IntrusivePtr& other) noexcept
+		: IntrusivePtr(NewRef{}, other.get())
 		{
-		setPtr(other.get(), true);
 		}
 
 	template <class U, class = std::enable_if_t<std::is_convertible_v<U*, T*>>>
-	IntrusivePtr(IntrusivePtr<U> other) noexcept : ptr_(other.detach())
+	IntrusivePtr(IntrusivePtr<U> other) noexcept : ptr_(other.release())
 		{
 		// nop
 		}
@@ -98,7 +121,7 @@ public:
 	 * intrusive pointer to @c nullptr.
 	 * @returns the raw pointer without modifying the reference count.
 	 */
-	pointer detach() noexcept
+	pointer release() noexcept
 		{
 		auto result = ptr_;
 		if ( result )
@@ -106,21 +129,6 @@ public:
 		return result;
 		}
 
-	/**
-	 * Convenience function for assigning a new raw pointer. Equivalent to calling
-	 * @c operator= with an @c IntrusivePtr constructed from the arguments.
-	 * @param new_value Pointer to the new shared object.
-	 * @param add_ref Denotes whether the reference count of the new shared object
-	 *                shall be increased.
-	 */
-	void reset(pointer new_value = nullptr, bool add_ref = true) noexcept
-		{
-		auto old = ptr_;
-		setPtr(new_value, add_ref);
-		if ( old )
-			Unref(old);
-		}
-
 	IntrusivePtr& operator=(IntrusivePtr other) noexcept
 		{
 		swap(other);
@@ -160,7 +168,7 @@ private:
 			Ref(raw_ptr);
 		}
 
-	pointer ptr_;
+	pointer ptr_ = nullptr;
 };
 
 /**
@@ -175,7 +183,7 @@ template <class T, class... Ts>
 IntrusivePtr<T> make_intrusive(Ts&&... args)
 	{
 	// Assumes that objects start with a reference count of 1!
-	return {new T(std::forward<Ts>(args)...), false};
+	return {AdoptRef{}, new T(std::forward<Ts>(args)...)};
 	}
 
 // -- comparison to nullptr ----------------------------------------------------
@@ -246,24 +254,6 @@ bool operator!=(const T* x, const IntrusivePtr<T>& y) {
   return x != y.get();
 }
 
-/**
- * @relates IntrusivePtr
- */
-template <class T>
-bool operator<(const IntrusivePtr<T>& x, const T* y)
-	{
-	return x.get() < y;
-	}
-
-/**
- * @relates IntrusivePtr
- */
-template <class T>
-bool operator<(const T* x, const IntrusivePtr<T>& y)
-	{
-	return x < y.get();
-	}
-
 // -- comparison to intrusive pointer ------------------------------------------
 
 // Using trailing return type and decltype() here removes this function from
@@ -289,13 +279,3 @@ auto operator!=(const IntrusivePtr<T>& x, const IntrusivePtr<U>& y)
 	return x.get() != y.get();
 	}
 
-/**
- * @relates IntrusivePtr
- */
-template <class T>
-auto operator<(const IntrusivePtr<T>& x, const IntrusivePtr<T>& y)
--> decltype(x.get() < y.get())
-	{
-	return x.get() < y.get();
-	}
-

src/OpaqueVal.cc

@@ -791,10 +791,9 @@ bool BloomFilterVal::Typify(BroType* arg_type)
 	type = arg_type;
 	type->Ref();
 
-	TypeList* tl = new TypeList(type);
+	auto tl = make_intrusive<TypeList>(type);
 	tl->Append(type->Ref());
-	hash = new CompositeHash(tl);
+	hash = new CompositeHash(std::move(tl));
-	Unref(tl);
 
 	return true;
 	}
@@ -963,10 +962,9 @@ bool CardinalityVal::Typify(BroType* arg_type)
 	type = arg_type;
 	type->Ref();
 
-	TypeList* tl = new TypeList(type);
+	auto tl = make_intrusive<TypeList>(type);
 	tl->Append(type->Ref());
-	hash = new CompositeHash(tl);
+	hash = new CompositeHash(std::move(tl));
-	Unref(tl);
 
 	return true;
 	}

src/OpaqueVal.h

@@ -160,10 +160,10 @@ namespace probabilistic {
 
 class HashVal : public OpaqueVal {
 public:
-	virtual bool IsValid() const;
+	bool IsValid() const;
-	virtual bool Init();
+	bool Init();
-	virtual bool Feed(const void* data, size_t size);
+	bool Feed(const void* data, size_t size);
-	virtual StringVal* Get();
+	StringVal* Get();
 
 protected:
 	HashVal()	{ valid = false; }

src/Stmt.cc

@@ -207,7 +207,7 @@ static IntrusivePtr<EnumVal> lookup_enum_val(const char* module_name, const char
 
 	int index = et->Lookup(module_name, name);
 	assert(index >= 0);
-	IntrusivePtr<EnumVal> rval{et->GetVal(index), false};
+	IntrusivePtr<EnumVal> rval{AdoptRef{}, et->GetVal(index)};
 
 	return rval;
 	}
@@ -226,7 +226,7 @@ static Val* print_log(val_list* vals)
 		}
 
 	record->Assign(0, new Val(current_time(), TYPE_TIME));
-	record->Assign(1, vec.detach());
+	record->Assign(1, vec.release());
 	log_mgr->Write(plval.get(), record.get());
 	return nullptr;
 	}
@@ -602,10 +602,9 @@ static void int_del_func(void* v)
 
 void SwitchStmt::Init()
 	{
-	TypeList* t = new TypeList();
+	auto t = make_intrusive<TypeList>();
 	t->Append(e->Type()->Ref());
-	comp_hash = new CompositeHash(t);
+	comp_hash = new CompositeHash(std::move(t));
-	Unref(t);
 
 	case_label_value_map.SetDeleteFunc(int_del_func);
 	}

src/Val.cc

@@ -1352,7 +1352,8 @@ void TableVal::Init(TableType* t)
 	else
 		subnets = 0;
 
-	table_hash = new CompositeHash(table_type->Indices());
+	table_hash = new CompositeHash(IntrusivePtr<TypeList>(NewRef{},
+	                               table_type->Indices()));
 	val.table_val = new PDict<TableEntryVal>;
 	val.table_val->SetDeleteFunc(table_entry_val_delete_func);
 	}
@@ -1984,7 +1985,7 @@ void TableVal::CallChangeFunc(const Val* index, Val* old_value, OnChangeType tpe
 
 	try
 		{
-		IntrusivePtr<Val> thefunc{change_func->Eval(nullptr), false};
+		IntrusivePtr<Val> thefunc{AdoptRef{}, change_func->Eval(nullptr)};
 
 		if ( ! thefunc )
 			{
@@ -3273,7 +3274,7 @@ Val* cast_value_to_type(Val* v, BroType* t)
 		if ( ! dv )
 			return 0;
 
-		return static_cast<bro_broker::DataVal *>(dv)->castTo(t).detach();
+		return static_cast<bro_broker::DataVal *>(dv)->castTo(t).release();
 		}
 
 	return 0;

src/broker/Data.cc

@@ -252,14 +252,14 @@ struct val_converter {
 				if ( ! index_val )
 					return nullptr;
 
-				list_val->Append(index_val.detach());
+				list_val->Append(index_val.release());
 				}
 
 
 			rval->Assign(list_val.get(), nullptr);
 			}
 
-		return rval.detach();
+		return rval.release();
 		}
 
 	result_type operator()(broker::table& a)
@@ -312,7 +312,7 @@ struct val_converter {
 				if ( ! index_val )
 					return nullptr;
 
-				list_val->Append(index_val.detach());
+				list_val->Append(index_val.release());
 				}
 
 			auto value_val = bro_broker::data_to_val(move(item.second),
@@ -321,10 +321,10 @@ struct val_converter {
 			if ( ! value_val )
 				return nullptr;
 
-			rval->Assign(list_val.get(), value_val.detach());
+			rval->Assign(list_val.get(), value_val.release());
 			}
 
-		return rval.detach();
+		return rval.release();
 		}
 
 	result_type operator()(broker::vector& a)
@@ -341,10 +341,10 @@ struct val_converter {
 				if ( ! item_val )
 					return nullptr;
 
-				rval->Assign(rval->Size(), item_val.detach());
+				rval->Assign(rval->Size(), item_val.release());
 				}
 
-			return rval.detach();
+			return rval.release();
 			}
 		else if ( type->Tag() == TYPE_FUNC )
 			{
@@ -410,11 +410,11 @@ struct val_converter {
 				if ( ! item_val )
 					return nullptr;
 
-				rval->Assign(i, item_val.detach());
+				rval->Assign(i, item_val.release());
 				++idx;
 				}
 
-			return rval.detach();
+			return rval.release();
 			}
 		else if ( type->Tag() == TYPE_PATTERN )
 			{
@@ -792,9 +792,9 @@ static bool data_type_check(const broker::data& d, BroType* t)
 IntrusivePtr<Val> bro_broker::data_to_val(broker::data d, BroType* type)
 	{
 	if ( type->Tag() == TYPE_ANY )
-		return {bro_broker::make_data_val(move(d)), false};
+		return {AdoptRef{}, bro_broker::make_data_val(move(d))};
 
-	return {caf::visit(val_converter{type}, std::move(d)), false};
+	return {AdoptRef{}, caf::visit(val_converter{type}, std::move(d))};
 	}
 
 broker::expected<broker::data> bro_broker::val_to_data(const Val* v)

src/broker/Manager.cc

@@ -998,7 +998,7 @@ void Manager::ProcessEvent(const broker::topic& topic, broker::zeek::Event ev)
 		auto val = data_to_val(std::move(args[i]), expected_type);
 
 		if ( val )
-			vl.push_back(val.detach());
+			vl.push_back(val.release());
 		else
 			{
 			auto expected_name = type_name(expected_type->Tag());
@@ -1210,7 +1210,7 @@ bool Manager::ProcessIdentifierUpdate(broker::zeek::IdentifierUpdate iu)
 		return false;
 		}
 
-	id->SetVal(val.detach());
+	id->SetVal(val.release());
 	return true;
 	}
 

src/file_analysis/AnalyzerSet.cc

@@ -20,11 +20,10 @@ static void analyzer_del_func(void* v)
 
 AnalyzerSet::AnalyzerSet(File* arg_file) : file(arg_file)
 	{
-	TypeList* t = new TypeList();
+	auto t = make_intrusive<TypeList>();
 	t->Append(file_mgr->GetTagEnumType()->Ref());
 	t->Append(BifType::Record::Files::AnalyzerArgs->Ref());
-	analyzer_hash = new CompositeHash(t);
+	analyzer_hash = new CompositeHash(std::move(t));
-	Unref(t);
 	analyzer_map.SetDeleteFunc(analyzer_del_func);
 	}
 

src/probabilistic/Topk.cc

@@ -27,10 +27,9 @@ void TopkVal::Typify(BroType* t)
 	{
 	assert(!hash && !type);
 	type = t->Ref();
-	TypeList* tl = new TypeList(t);
+	auto tl = make_intrusive<TypeList>(t);
 	tl->Append(t->Ref());
-	hash = new CompositeHash(tl);
+	hash = new CompositeHash(std::move(tl));
-	Unref(tl);
 	}
 
 HashKey* TopkVal::GetHash(Val* v) const
@@ -514,7 +513,7 @@ bool TopkVal::DoUnserialize(const broker::data& data)
 
 			Element* e = new Element();
 			e->epsilon = *epsilon;
-			e->value = val.detach();
+			e->value = val.release();
 			e->parent = b;
 
 			b->elements.insert(b->elements.end(), e);

src/supervisor/Supervisor.cc

@@ -1024,7 +1024,7 @@ Supervisor::NodeConfig Supervisor::NodeConfig::FromRecord(const RecordVal* node)
 
 	while ( (v = cluster_table->NextEntry(k, c)) )
 		{
-		IntrusivePtr<ListVal> key{cluster_table_val->RecoverIndex(k), false};
+		IntrusivePtr<ListVal> key{AdoptRef{}, cluster_table_val->RecoverIndex(k)};
 		delete k;
 		auto name = key->Index(0)->AsStringVal()->ToStdString();
 		auto rv = v->Value()->AsRecordVal();
@@ -1100,7 +1100,7 @@ std::string Supervisor::NodeConfig::ToJSON() const
 	{
 	auto re = std::make_unique<RE_Matcher>("^_");
 	auto node_val = ToRecord();
-	IntrusivePtr<StringVal> json_val{node_val->ToJSON(false, re.get()), false};
+	IntrusivePtr<StringVal> json_val{AdoptRef{}, node_val->ToJSON(false, re.get())};
 	auto rval = json_val->ToStdString();
 	return rval;
 	}
@@ -1152,7 +1152,7 @@ IntrusivePtr<RecordVal> Supervisor::NodeConfig::ToRecord() const
 		if ( ep.interface )
 			val->Assign(ept->FieldOffset("interface"), new StringVal(*ep.interface));
 
-		cluster_val->Assign(key.get(), val.detach());
+		cluster_val->Assign(key.get(), val.release());
 		}
 
 	return rval;
@@ -1163,7 +1163,7 @@ IntrusivePtr<RecordVal> Supervisor::Node::ToRecord() const
 	auto rt = BifType::Record::Supervisor::NodeStatus;
 	auto rval = make_intrusive<RecordVal>(rt);
 
-	rval->Assign(rt->FieldOffset("node"), config.ToRecord().detach());
+	rval->Assign(rt->FieldOffset("node"), config.ToRecord().release());
 
 	if ( pid )
 		rval->Assign(rt->FieldOffset("pid"), val_mgr->GetInt(pid));
@@ -1230,7 +1230,7 @@ bool Supervisor::SupervisedNode::InitCluster() const
 			val->Assign(cluster_node_type->FieldOffset("manager"),
 			            new StringVal(*manager_name));
 
-		cluster_nodes->Assign(key.get(), val.detach());
+		cluster_nodes->Assign(key.get(), val.release());
 		}
 
 	cluster_manager_is_logger_id->SetVal(val_mgr->GetBool(! has_logger));
@@ -1329,7 +1329,7 @@ RecordVal* Supervisor::Status(std::string_view node_name)
 			const auto& node = n.second;
 			auto key = make_intrusive<StringVal>(name);
 			auto val = node.ToRecord();
-			node_table_val->Assign(key.get(), val.detach());
+			node_table_val->Assign(key.get(), val.release());
 			}
 		}
 	else
@@ -1343,7 +1343,7 @@ RecordVal* Supervisor::Status(std::string_view node_name)
 		const auto& node = it->second;
 		auto key = make_intrusive<StringVal>(name);
 		auto val = node.ToRecord();
-		node_table_val->Assign(key.get(), val.detach());
+		node_table_val->Assign(key.get(), val.release());
 		}
 
 	return rval;

src/supervisor/supervisor.bif

@@ -87,11 +87,11 @@ function Supervisor::__node%(%): Supervisor::NodeConfig
 		auto rt = BifType::Record::Supervisor::NodeConfig;
 		auto rval = make_intrusive<RecordVal>(rt);
 		rval->Assign(rt->FieldOffset("name"), new StringVal("<invalid>"));
-		return rval.detach();
+		return rval.release();
 		}
 
 	auto rval = zeek::Supervisor::ThisNode()->config.ToRecord();
-	return rval.detach();
+	return rval.release();
 	%}
 
 function Supervisor::__is_supervisor%(%): bool

src/zeek.bif

@@ -400,7 +400,7 @@ function terminate%(%): bool
 // is false).
 static bool prepare_environment(TableVal* tbl, bool set)
 	{
-	IntrusivePtr<ListVal> idxs{tbl->ConvertToPureList(), false};
+	IntrusivePtr<ListVal> idxs{AdoptRef{}, tbl->ConvertToPureList()};
 
 	for ( int i = 0; i < idxs->Length(); ++i )
 		{
@@ -1868,7 +1868,7 @@ function zeek_args%(%): string_vec
 	for ( auto i = 0; i < bro_argc; ++i )
 		rval->Assign(rval->Size(), new StringVal(bro_argv[i]));
 
-	return rval.detach();
+	return rval.release();
 	%}
 
 ## Checks whether Zeek reads traffic from one or more network interfaces (as
@@ -1912,7 +1912,7 @@ function packet_source%(%): PacketSource
 		r->Assign(3, val_mgr->GetCount(ps->Netmask()));
 		}
 
-	return r.detach();
+	return r.release();
 	%}
 
 ## Generates a table of the size of all global variables. The table index is