diff --git a/CHANGES b/CHANGES index 6d7c41cc30..95b706e366 100644 --- a/CHANGES +++ b/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) diff --git a/VERSION b/VERSION index 310ecc777d..236d2ff796 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.2.0-dev.71 +3.2.0-dev.80 diff --git a/src/CompHash.cc b/src/CompHash.cc index 37ee7e25c9..d1458a1ec9 100644 --- a/src/CompHash.cc +++ b/src/CompHash.cc @@ -13,10 +13,9 @@ #include #include -CompositeHash::CompositeHash(TypeList* composite_type) +CompositeHash::CompositeHash(IntrusivePtr 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; } diff --git a/src/CompHash.h b/src/CompHash.h index ef0f0d14c3..9aa3555200 100644 --- a/src/CompHash.h +++ b/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 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 type; char* key; // space for composite key int size; int is_singleton; // if just one type in index diff --git a/src/Expr.cc b/src/Expr.cc index 70080bc0cb..2b4839c79a 100644 --- a/src/Expr.cc +++ b/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; diff --git a/src/Frame.cc b/src/Frame.cc index 01bba1a679..8db8a1f7ff 100644 --- a/src/Frame.cc +++ b/src/Frame.cc @@ -485,7 +485,7 @@ std::pair 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); diff --git a/src/IntrusivePtr.h b/src/IntrusivePtr.h index 99a19f80ff..767aae197c 100644 --- a/src/IntrusivePtr.h +++ b/src/IntrusivePtr.h @@ -5,6 +5,18 @@ #include #include +/** + * 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) - { - // nop - } + constexpr IntrusivePtr() noexcept = default; 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 >> - IntrusivePtr(IntrusivePtr other) noexcept : ptr_(other.detach()) + IntrusivePtr(IntrusivePtr 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 IntrusivePtr make_intrusive(Ts&&... args) { // Assumes that objects start with a reference count of 1! - return {new T(std::forward(args)...), false}; + return {AdoptRef{}, new T(std::forward(args)...)}; } // -- comparison to nullptr ---------------------------------------------------- @@ -246,24 +254,6 @@ bool operator!=(const T* x, const IntrusivePtr& y) { return x != y.get(); } -/** - * @relates IntrusivePtr - */ -template -bool operator<(const IntrusivePtr& x, const T* y) - { - return x.get() < y; - } - -/** - * @relates IntrusivePtr - */ -template -bool operator<(const T* x, const IntrusivePtr& 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& x, const IntrusivePtr& y) return x.get() != y.get(); } -/** - * @relates IntrusivePtr - */ -template -auto operator<(const IntrusivePtr& x, const IntrusivePtr& y) --> decltype(x.get() < y.get()) - { - return x.get() < y.get(); - } - diff --git a/src/OpaqueVal.cc b/src/OpaqueVal.cc index 460fea1945..96f67f55a8 100644 --- a/src/OpaqueVal.cc +++ b/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(type); tl->Append(type->Ref()); - hash = new CompositeHash(tl); - Unref(tl); + hash = new CompositeHash(std::move(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(type); tl->Append(type->Ref()); - hash = new CompositeHash(tl); - Unref(tl); + hash = new CompositeHash(std::move(tl)); return true; } diff --git a/src/OpaqueVal.h b/src/OpaqueVal.h index 7d216e7ff9..835bc0bb3f 100644 --- a/src/OpaqueVal.h +++ b/src/OpaqueVal.h @@ -160,10 +160,10 @@ namespace probabilistic { class HashVal : public OpaqueVal { public: - virtual bool IsValid() const; - virtual bool Init(); - virtual bool Feed(const void* data, size_t size); - virtual StringVal* Get(); + bool IsValid() const; + bool Init(); + bool Feed(const void* data, size_t size); + StringVal* Get(); protected: HashVal() { valid = false; } diff --git a/src/Stmt.cc b/src/Stmt.cc index 39999058ca..76b31c07e4 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -207,7 +207,7 @@ static IntrusivePtr lookup_enum_val(const char* module_name, const char int index = et->Lookup(module_name, name); assert(index >= 0); - IntrusivePtr rval{et->GetVal(index), false}; + IntrusivePtr 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(); t->Append(e->Type()->Ref()); - comp_hash = new CompositeHash(t); - Unref(t); + comp_hash = new CompositeHash(std::move(t)); case_label_value_map.SetDeleteFunc(int_del_func); } diff --git a/src/Val.cc b/src/Val.cc index 595c208f8a..66462e5e79 100644 --- a/src/Val.cc +++ b/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(NewRef{}, + table_type->Indices())); val.table_val = new PDict; 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 thefunc{change_func->Eval(nullptr), false}; + IntrusivePtr 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(dv)->castTo(t).detach(); + return static_cast(dv)->castTo(t).release(); } return 0; diff --git a/src/broker/Data.cc b/src/broker/Data.cc index 8733f1b686..513d13bfd6 100644 --- a/src/broker/Data.cc +++ b/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 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 bro_broker::val_to_data(const Val* v) diff --git a/src/broker/Manager.cc b/src/broker/Manager.cc index eae0ce723f..0752032e51 100644 --- a/src/broker/Manager.cc +++ b/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; } diff --git a/src/file_analysis/AnalyzerSet.cc b/src/file_analysis/AnalyzerSet.cc index d3cbe300f1..53c677eff3 100644 --- a/src/file_analysis/AnalyzerSet.cc +++ b/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(); t->Append(file_mgr->GetTagEnumType()->Ref()); t->Append(BifType::Record::Files::AnalyzerArgs->Ref()); - analyzer_hash = new CompositeHash(t); - Unref(t); + analyzer_hash = new CompositeHash(std::move(t)); analyzer_map.SetDeleteFunc(analyzer_del_func); } diff --git a/src/probabilistic/Topk.cc b/src/probabilistic/Topk.cc index f76b3f4086..c177ba4545 100644 --- a/src/probabilistic/Topk.cc +++ b/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(t); tl->Append(t->Ref()); - hash = new CompositeHash(tl); - Unref(tl); + hash = new CompositeHash(std::move(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); diff --git a/src/supervisor/Supervisor.cc b/src/supervisor/Supervisor.cc index a1dd2a21d4..b790eeead8 100644 --- a/src/supervisor/Supervisor.cc +++ b/src/supervisor/Supervisor.cc @@ -1024,7 +1024,7 @@ Supervisor::NodeConfig Supervisor::NodeConfig::FromRecord(const RecordVal* node) while ( (v = cluster_table->NextEntry(k, c)) ) { - IntrusivePtr key{cluster_table_val->RecoverIndex(k), false}; + IntrusivePtr 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("^_"); auto node_val = ToRecord(); - IntrusivePtr json_val{node_val->ToJSON(false, re.get()), false}; + IntrusivePtr json_val{AdoptRef{}, node_val->ToJSON(false, re.get())}; auto rval = json_val->ToStdString(); return rval; } @@ -1152,7 +1152,7 @@ IntrusivePtr 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 Supervisor::Node::ToRecord() const auto rt = BifType::Record::Supervisor::NodeStatus; auto rval = make_intrusive(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(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(name); auto val = node.ToRecord(); - node_table_val->Assign(key.get(), val.detach()); + node_table_val->Assign(key.get(), val.release()); } return rval; diff --git a/src/supervisor/supervisor.bif b/src/supervisor/supervisor.bif index 6bc6db17d6..b297c2944f 100644 --- a/src/supervisor/supervisor.bif +++ b/src/supervisor/supervisor.bif @@ -87,11 +87,11 @@ function Supervisor::__node%(%): Supervisor::NodeConfig auto rt = BifType::Record::Supervisor::NodeConfig; auto rval = make_intrusive(rt); rval->Assign(rt->FieldOffset("name"), new StringVal("")); - return rval.detach(); + return rval.release(); } auto rval = zeek::Supervisor::ThisNode()->config.ToRecord(); - return rval.detach(); + return rval.release(); %} function Supervisor::__is_supervisor%(%): bool diff --git a/src/zeek.bif b/src/zeek.bif index 5f041e44a5..ffa5acee02 100644 --- a/src/zeek.bif +++ b/src/zeek.bif @@ -400,7 +400,7 @@ function terminate%(%): bool // is false). static bool prepare_environment(TableVal* tbl, bool set) { - IntrusivePtr idxs{tbl->ConvertToPureList(), false}; + IntrusivePtr 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