diff --git a/src/Expr.cc b/src/Expr.cc index 4c1492e2d1..68e10a9a90 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -273,7 +273,7 @@ void NameExpr::Assign(Frame* f, IntrusivePtr v) if ( id->IsGlobal() ) id->SetVal(std::move(v)); else - f->SetElement(id.get(), v.release()); + f->SetElement(id.get(), std::move(v)); } bool NameExpr::IsPure() const diff --git a/src/Frame.cc b/src/Frame.cc index 70eb1118d3..2217dea187 100644 --- a/src/Frame.cc +++ b/src/Frame.cc @@ -49,7 +49,7 @@ Frame::~Frame() Unref(i); for ( int i = 0; i < size; ++i ) - UnrefElement(i); + ClearElement(i); delete [] weak_refs; } @@ -64,37 +64,41 @@ void Frame::AddFunctionWithClosureRef(BroFunc* func) functions_with_closure_frame_reference->emplace_back(func); } -void Frame::SetElement(int n, Val* v, bool weak_ref) +void Frame::SetElement(int n, Val* v) + { SetElement(n, {AdoptRef{}, v}); } + +void Frame::SetElement(int n, IntrusivePtr v) { - UnrefElement(n); - frame[n] = {AdoptRef{}, v}; + ClearElement(n); + frame[n] = std::move(v); - if ( weak_ref ) - { - if ( ! weak_refs ) - { - weak_refs = new bool[size]; - - for ( auto i = 0; i < size; ++i ) - weak_refs[i] = false; - } - - weak_refs[n] = true; - } - else - { - if ( weak_refs ) - weak_refs[n] = false; - } + if ( weak_refs ) + weak_refs[n] = false; } -void Frame::SetElement(const ID* id, Val* v) +void Frame::SetElementWeak(int n, Val* v) + { + ClearElement(n); + frame[n] = {AdoptRef{}, v}; + + if ( ! weak_refs ) + { + weak_refs = new bool[size]; + + for ( auto i = 0; i < size; ++i ) + weak_refs[i] = false; + } + + weak_refs[n] = true; + } + +void Frame::SetElement(const ID* id, IntrusivePtr v) { if ( closure ) { if ( IsOuterID(id) ) { - closure->SetElement(id, v); + closure->SetElement(id, std::move(v)); return; } } @@ -110,11 +114,11 @@ void Frame::SetElement(const ID* id, Val* v) // id->Offset() below is otherwise responsible for keeping track // of the implied reference count of the passed-in 'v' argument. // i.e. if we end up storing it twice, we need an addition Ref. - SetElement(where->second, v->Ref()); + SetElement(where->second, v); } } - SetElement(id->Offset(), v); + SetElement(id->Offset(), std::move(v)); } Val* Frame::GetElement(const ID* id) const @@ -140,7 +144,7 @@ void Frame::Reset(int startIdx) { for ( int i = startIdx; i < size; ++i ) { - UnrefElement(i); + ClearElement(i); frame[i] = nullptr; } } @@ -195,9 +199,7 @@ static bool val_is_func(const IntrusivePtr& v, BroFunc* func) return v->AsFunc() == func; } -static void clone_if_not_func(const std::unique_ptr[]>& frame, - int offset, BroFunc* func, - Frame* other) +void Frame::CloneNonFuncElement(int offset, BroFunc* func, Frame* other) const { const auto& v = frame[offset]; @@ -206,12 +208,12 @@ static void clone_if_not_func(const std::unique_ptr[]>& frame, if ( val_is_func(v, func) ) { - other->SetElement(offset, v.get(), true); + other->SetElementWeak(offset, v.get()); return; } auto rval = v->Clone(); - other->SetElement(offset, rval.release()); + other->SetElement(offset, std::move(rval)); } Frame* Frame::SelectiveClone(const id_list& selection, BroFunc* func) const @@ -240,7 +242,7 @@ Frame* Frame::SelectiveClone(const id_list& selection, BroFunc* func) const auto where = offset_map->find(std::string(id->Name())); if ( where != offset_map->end() ) { - clone_if_not_func(frame, where->second, func, other); + CloneNonFuncElement(where->second, func, other); continue; } } @@ -248,7 +250,7 @@ Frame* Frame::SelectiveClone(const id_list& selection, BroFunc* func) const if ( ! frame[id->Offset()] ) reporter->InternalError("Attempted to clone an id ('%s') with no associated value.", id->Name()); - clone_if_not_func(frame, id->Offset(), func, other); + CloneNonFuncElement(id->Offset(), func, other); } /** @@ -531,7 +533,7 @@ void Frame::ClearTrigger() trigger = nullptr; } -void Frame::UnrefElement(int n) +void Frame::ClearElement(int n) { if ( weak_refs && weak_refs[n] ) frame[n].release(); diff --git a/src/Frame.h b/src/Frame.h index 3880159aec..57827f1b5b 100644 --- a/src/Frame.h +++ b/src/Frame.h @@ -45,16 +45,14 @@ public: Val* NthElement(int n) const { return frame[n].get(); } /** - * Sets the element at index *n* of the underlying array - * to *v*. - * + * Sets the element at index *n* of the underlying array to *v*. * @param n the index to set * @param v the value to set it to - * @param weak_ref whether the frame owns the value and should unref - * it upon destruction. Used to break circular references between - * lambda functions and closure frames. */ - void SetElement(int n, Val* v, bool weak_ref = false); + void SetElement(int n, IntrusivePtr v); + + [[deprecated("Remove in v4.1. Pass IntrusivePtr instead.")]] + void SetElement(int n, Val* v); /** * Associates *id* and *v* in the frame. Future lookups of @@ -63,7 +61,7 @@ public: * @param id the ID to associate * @param v the value to associate it with */ - void SetElement(const ID* id, Val* v); + void SetElement(const ID* id, IntrusivePtr v); /** * Gets the value associated with *id* and returns it. Returns @@ -76,8 +74,7 @@ public: /** * Resets all of the indexes from [*startIdx, frame_size) in - * the Frame. Unrefs all of the values in reset indexes. - * + * the Frame. * @param the first index to unref. */ void Reset(int startIdx); @@ -232,9 +229,27 @@ private: using OffsetMap = std::unordered_map; /** - * Unrefs the value at offset 'n' frame unless it's a weak reference. + * Sets the element at index *n* of the underlying array to *v*, but does + * not take ownership of a reference count to it. This method is used to + * break circular references between lambda functions and closure frames. + * @param n the index to set + * @param v the value to set it to (caller has not Ref'd and Frame will + * not Unref it) */ - void UnrefElement(int n); + void SetElementWeak(int n, Val* v); + + /** + * Clone an element at an offset into other frame if not equal to a given + * function (in that case just assigna weak reference). Used to break + * circular references between lambda functions and closure frames. + */ + void CloneNonFuncElement(int offset, BroFunc* func, Frame* other) const; + + /** + * Resets the value at offset 'n' frame (by decrementing reference + * count if not a weak reference). + */ + void ClearElement(int n); /** Have we captured this id? */ bool IsOuterID(const ID* in) const; diff --git a/src/Func.cc b/src/Func.cc index ec30fcc2b0..8553d9234f 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -365,11 +365,11 @@ IntrusivePtr BroFunc::operator()(zeek::Args* args, Frame* parent) const // Fill in the rest of the frame with the function's arguments. for ( auto j = 0u; j < args->size(); ++j ) { - Val* arg = (*args)[j].get(); + const auto& arg = (*args)[j]; if ( f->NthElement(j) != arg ) // Either not yet set, or somebody reassigned the frame slot. - f->SetElement(j, arg->Ref()); + f->SetElement(j, arg); } f->Reset(args->size()); diff --git a/src/Stmt.cc b/src/Stmt.cc index fde3cdc5fa..0c2e3235d7 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -811,7 +811,7 @@ IntrusivePtr SwitchStmt::DoExec(Frame* f, Val* v, stmt_flow_type& flow) con if ( matching_id ) { auto cv = cast_value_to_type(v, matching_id->GetType().get()); - f->SetElement(matching_id, cv.release()); + f->SetElement(matching_id, std::move(cv)); } flow = FLOW_NEXT; @@ -1191,10 +1191,10 @@ IntrusivePtr ForStmt::DoExec(Frame* f, Val* v, stmt_flow_type& flow) const delete k; if ( value_var ) - f->SetElement(value_var.get(), current_tev->GetVal()->Ref()); + f->SetElement(value_var.get(), current_tev->GetVal()); for ( int i = 0; i < ind_lv->Length(); i++ ) - f->SetElement((*loop_vars)[i], ind_lv->Idx(i)->Ref()); + f->SetElement((*loop_vars)[i], ind_lv->Idx(i)); flow = FLOW_NEXT; @@ -1230,7 +1230,7 @@ IntrusivePtr ForStmt::DoExec(Frame* f, Val* v, stmt_flow_type& flow) const // Set the loop variable to the current index, and make // another pass over the loop body. - f->SetElement((*loop_vars)[0], val_mgr->Count(i).release()); + f->SetElement((*loop_vars)[0], val_mgr->Count(i)); flow = FLOW_NEXT; ret = body->Exec(f, flow); @@ -1244,8 +1244,8 @@ IntrusivePtr ForStmt::DoExec(Frame* f, Val* v, stmt_flow_type& flow) const for ( int i = 0; i < sval->Len(); ++i ) { - f->SetElement((*loop_vars)[0], - new StringVal(1, (const char*) sval->Bytes() + i)); + auto sv = make_intrusive(1, (const char*) sval->Bytes() + i); + f->SetElement((*loop_vars)[0], std::move(sv)); flow = FLOW_NEXT; ret = body->Exec(f, flow); @@ -1653,23 +1653,24 @@ IntrusivePtr InitStmt::Exec(Frame* f, stmt_flow_type& flow) const { const auto& t = aggr->GetType(); - Val* v = nullptr; + IntrusivePtr v; switch ( t->Tag() ) { case TYPE_RECORD: - v = new RecordVal(cast_intrusive(t)); + v = make_intrusive(cast_intrusive(t)); break; case TYPE_VECTOR: - v = new VectorVal(cast_intrusive(t)); + v = make_intrusive(cast_intrusive(t)); break; case TYPE_TABLE: - v = new TableVal(cast_intrusive(t), {NewRef{}, aggr->Attrs()}); + v = make_intrusive(cast_intrusive(t), + IntrusivePtr{NewRef{}, aggr->Attrs()}); break; default: break; } - f->SetElement(aggr, v); + f->SetElement(aggr, std::move(v)); } return nullptr;