Switch Frame::SetElement() to use IntrusivePtr

src/Expr.cc

@@ -273,7 +273,7 @@ void NameExpr::Assign(Frame* f, IntrusivePtr<Val> 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

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,13 +64,23 @@ 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<Val> v)
 	{
-	UnrefElement(n);
+	ClearElement(n);
+	frame[n] = std::move(v);
+
+	if ( weak_refs )
+		weak_refs[n] = false;
+	}
+
+void Frame::SetElementWeak(int n, Val* v)
+	{
+	ClearElement(n);
 	frame[n] = {AdoptRef{}, v};
 
-	if ( weak_ref )
-		{
 	if ( ! weak_refs )
 		{
 		weak_refs = new bool[size];
@@ -81,20 +91,14 @@ void Frame::SetElement(int n, Val* v, bool weak_ref)
 
 	weak_refs[n] = true;
 	}
-	else
-		{
-		if ( weak_refs )
-			weak_refs[n] = false;
-		}
-	}
 
-void Frame::SetElement(const ID* id, Val* v)
+void Frame::SetElement(const ID* id, IntrusivePtr<Val> 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<Val>& v, BroFunc* func)
 	return v->AsFunc() == func;
 	}
 
-static void clone_if_not_func(const std::unique_ptr<IntrusivePtr<Val>[]>& frame,
+void Frame::CloneNonFuncElement(int offset, BroFunc* func, Frame* other) const
-                              int offset, BroFunc* func,
-                              Frame* other)
 	{
 	const auto& v = frame[offset];
 
@@ -206,12 +208,12 @@ static void clone_if_not_func(const std::unique_ptr<IntrusivePtr<Val>[]>& 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();

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
+	 * Sets the element at index *n* of the underlying array to *v*.
-	 * 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<Val> 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<Val> 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<std::string, int>;
 
 	/**
-	 * 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;

src/Func.cc

@@ -365,11 +365,11 @@ IntrusivePtr<Val> 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());

src/Stmt.cc

@@ -811,7 +811,7 @@ IntrusivePtr<Val> 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<Val> 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<Val> 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<Val> ForStmt::DoExec(Frame* f, Val* v, stmt_flow_type& flow) const
 
 		for ( int i = 0; i < sval->Len(); ++i )
 			{
-			f->SetElement((*loop_vars)[0],
+			auto sv = make_intrusive<StringVal>(1, (const char*) sval->Bytes() + i);
-					new StringVal(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<Val> InitStmt::Exec(Frame* f, stmt_flow_type& flow) const
 		{
 		const auto& t = aggr->GetType();
 
-		Val* v = nullptr;
+		IntrusivePtr<Val> v;
 
 		switch ( t->Tag() ) {
 		case TYPE_RECORD:
-			v = new RecordVal(cast_intrusive<RecordType>(t));
+			v = make_intrusive<RecordVal>(cast_intrusive<RecordType>(t));
 			break;
 		case TYPE_VECTOR:
-			v = new VectorVal(cast_intrusive<VectorType>(t));
+			v = make_intrusive<VectorVal>(cast_intrusive<VectorType>(t));
 			break;
 		case TYPE_TABLE:
-			v = new TableVal(cast_intrusive<TableType>(t), {NewRef{}, aggr->Attrs()});
+			v = make_intrusive<TableVal>(cast_intrusive<TableType>(t),
+			                             IntrusivePtr{NewRef{}, aggr->Attrs()});
 			break;
 		default:
 			break;
 		}
 
-		f->SetElement(aggr, v);
+		f->SetElement(aggr, std::move(v));
 		}
 
 	return nullptr;