Address PR review feedback on zam-feature-complete

* insert_or_assign usage
  * master -> primary
  * FunctionIngredientsPtr
  * FuncType::Capture deprecations
  * no new ScriptFunc constructor

src/Expr.cc

@@ -4645,8 +4645,8 @@ void CallExpr::ExprDescribe(ODesc* d) const
 		args->Describe(d);
 	}
 
-LambdaExpr::LambdaExpr(std::shared_ptr<FunctionIngredients> arg_ing, IDPList arg_outer_ids,
-                       std::string name, StmtPtr when_parent)
+LambdaExpr::LambdaExpr(FunctionIngredientsPtr arg_ing, IDPList arg_outer_ids, std::string name,
+                       StmtPtr when_parent)
 	: Expr(EXPR_LAMBDA)
 	{
 	ingredients = std::move(arg_ing);
@@ -4661,17 +4661,17 @@ LambdaExpr::LambdaExpr(std::shared_ptr<FunctionIngredients> arg_ing, IDPList arg
 		return;
 		}
 
-	// Install a master version of the function globally.  This is used
+	// Install a primary version of the function globally.  This is used
 	// by both broker (for transmitting closures) and script optimization
 	// (replacing its AST body with a compiled one).
-	master_func = make_intrusive<ScriptFunc>(ingredients->GetID());
-	master_func->SetOuterIDs(outer_ids);
+	primary_func = make_intrusive<ScriptFunc>(ingredients->GetID());
+	primary_func->SetOuterIDs(outer_ids);
 
 	// When we build the body, it will get updated with initialization
 	// statements.  Update the ingredients to reflect the new body,
 	// and no more need for initializers.
-	master_func->AddBody(*ingredients);
-	master_func->SetScope(ingredients->Scope());
+	primary_func->AddBody(*ingredients);
+	primary_func->SetScope(ingredients->Scope());
 	ingredients->ClearInits();
 
 	if ( name.empty() )
@@ -4683,9 +4683,9 @@ LambdaExpr::LambdaExpr(std::shared_ptr<FunctionIngredients> arg_ing, IDPList arg
 	lambda_id = install_ID(my_name.c_str(), current_module.c_str(), true, false);
 
 	// Update lamb's name
-	master_func->SetName(my_name.c_str());
+	primary_func->SetName(my_name.c_str());
 
-	auto v = make_intrusive<FuncVal>(master_func);
+	auto v = make_intrusive<FuncVal>(primary_func);
 	lambda_id->SetVal(std::move(v));
 	lambda_id->SetType(ingr_t);
 	lambda_id->SetConst();
@@ -4697,7 +4697,7 @@ LambdaExpr::LambdaExpr(std::shared_ptr<FunctionIngredients> arg_ing, IDPList arg
 
 LambdaExpr::LambdaExpr(LambdaExpr* orig) : Expr(EXPR_LAMBDA)
 	{
-	master_func = orig->master_func;
+	primary_func = orig->primary_func;
 	ingredients = orig->ingredients;
 	lambda_id = orig->lambda_id;
 	my_name = orig->my_name;
@@ -4804,7 +4804,7 @@ void LambdaExpr::BuildName()
 	{
 	// Get the body's "string" representation.
 	ODesc d;
-	master_func->Describe(&d);
+	primary_func->Describe(&d);
 
 	for ( ;; )
 		{
@@ -4835,7 +4835,7 @@ ValPtr LambdaExpr::Eval(Frame* f) const
 	{
 	auto lamb = make_intrusive<ScriptFunc>(ingredients->GetID());
 
-	StmtPtr body = master_func->GetBodies()[0].stmts;
+	StmtPtr body = primary_func->GetBodies()[0].stmts;
 
 	if ( run_state::is_parsing )
 		// We're evaluating this lambda at parse time, which happens

src/Expr.h

@@ -32,6 +32,7 @@ class WhenInfo;
 using IDPtr = IntrusivePtr<ID>;
 using ScopePtr = IntrusivePtr<Scope>;
 using ScriptFuncPtr = IntrusivePtr<ScriptFunc>;
+using FunctionIngredientsPtr = std::shared_ptr<FunctionIngredients>;
 
 enum ExprTag : int
 	{
@@ -1454,8 +1455,8 @@ protected:
 class LambdaExpr final : public Expr
 	{
 public:
-	LambdaExpr(std::shared_ptr<FunctionIngredients> ingredients, IDPList outer_ids,
-	           std::string name = "", StmtPtr when_parent = nullptr);
+	LambdaExpr(FunctionIngredientsPtr ingredients, IDPList outer_ids, std::string name = "",
+	           StmtPtr when_parent = nullptr);
 
 	const std::string& Name() const { return my_name; }
 
@@ -1474,9 +1475,9 @@ public:
 	// Optimization-related:
 	ExprPtr Duplicate() override;
 
-	const ScriptFuncPtr& MasterFunc() const { return master_func; }
+	const ScriptFuncPtr& PrimaryFunc() const { return primary_func; }
 
-	const std::shared_ptr<FunctionIngredients>& Ingredients() const { return ingredients; }
+	const FunctionIngredientsPtr& Ingredients() const { return ingredients; }
 
 	bool IsReduced(Reducer* c) const override;
 	bool HasReducedOps(Reducer* c) const override;
@@ -1504,8 +1505,8 @@ private:
 
 	void UpdateCaptures(Reducer* c);
 
-	std::shared_ptr<FunctionIngredients> ingredients;
-	ScriptFuncPtr master_func;
+	FunctionIngredientsPtr ingredients;
+	ScriptFuncPtr primary_func;
 	IDPtr lambda_id;
 	IDPList outer_ids;
 	std::optional<CaptureList> captures;

src/Func.cc

@@ -295,12 +295,10 @@ void Func::CheckPluginResult(bool handled, const ValPtr& hook_result, FunctionFl
 namespace detail
 	{
 
-ScriptFunc::ScriptFunc(const IDPtr& id) : ScriptFunc::ScriptFunc(id.get()) { }
-
-ScriptFunc::ScriptFunc(const ID* id) : Func(SCRIPT_FUNC)
+ScriptFunc::ScriptFunc(const IDPtr& arg_id) : Func(SCRIPT_FUNC)
 	{
-	name = id->Name();
-	type = id->GetType<zeek::FuncType>();
+	name = arg_id->Name();
+	type = arg_id->GetType<zeek::FuncType>();
 	frame_size = 0;
 	}
 
@@ -337,7 +335,7 @@ ScriptFunc::~ScriptFunc()
 		{
 		auto& cvec = *captures_vec;
 		auto& captures = *type->GetCaptures();
-		for ( int i = 0; i < captures.size(); ++i )
+		for ( auto i = 0u; i < captures.size(); ++i )
 			if ( captures[i].IsManaged() )
 				ZVal::DeleteManagedType(cvec[i]);
 		}
@@ -562,7 +560,7 @@ void ScriptFunc::CreateCaptures(Frame* f)
 			captures_vec->push_back(ZVal());
 
 		if ( ! captures_vec )
-			(*captures_offset_mapping)[c.Id()->Name()] = offset;
+			captures_offset_mapping->insert_or_assign(c.Id()->Name(), offset);
 
 		++offset;
 		}
@@ -608,7 +606,7 @@ void ScriptFunc::SetCaptures(Frame* f)
 	int offset = 0;
 	for ( const auto& c : *captures )
 		{
-		(*captures_offset_mapping)[c.Id()->Name()] = offset;
+		captures_offset_mapping->insert_or_assign(c.Id()->Name(), offset);
 		++offset;
 		}
 	}
@@ -678,7 +676,7 @@ bool ScriptFunc::DeserializeCaptures(const broker::vector& data)
 		auto& captures = *type->GetCaptures();
 		int n = f->FrameSize();
 
-		ASSERT(captures.size() == n);
+		ASSERT(captures.size() == static_cast<size_t>(n));
 
 		auto cvec = std::make_unique<std::vector<ZVal>>();
 
@@ -721,17 +719,15 @@ FuncPtr ScriptFunc::DoClone()
 	if ( captures_vec )
 		{
 		auto cv_i = captures_vec->begin();
-		auto cv_copy = std::make_unique<std::vector<ZVal>>();
+		other->captures_vec = std::make_unique<std::vector<ZVal>>();
 		for ( auto& c : *type->GetCaptures() )
 			{
 			// Need to clone cv_i.
 			auto& t_i = c.Id()->GetType();
 			auto cv_i_val = cv_i->ToVal(t_i)->Clone();
-			cv_copy->push_back(ZVal(cv_i_val, t_i));
+			other->captures_vec->push_back(ZVal(cv_i_val, t_i));
 			++cv_i;
 			}
-
-		other->captures_vec = std::move(cv_copy);
 		}
 
 	return other;

src/Func.h

@@ -116,7 +116,10 @@ public:
 		}
 
 	// Various ways to add a new event handler to an existing function
-	// (event).
+	// (event).  The usual version to use is the first with its default
+	// parameter.  All of the others are for use by script optimization,
+	// as is a non-default second parameter to the first method, which
+	// overrides the function body in "ingr".
 	void AddBody(const detail::FunctionIngredients& ingr, detail::StmtPtr new_body = nullptr);
 	virtual void AddBody(detail::StmtPtr new_body, const std::vector<detail::IDPtr>& new_inits,
 	                     size_t new_frame_size, int priority,
@@ -172,7 +175,6 @@ class ScriptFunc : public Func
 	{
 public:
 	ScriptFunc(const IDPtr& id);
-	ScriptFunc(const ID* id);
 
 	// For compiled scripts.
 	ScriptFunc(std::string name, FuncTypePtr ft, std::vector<StmtPtr> bodies,
@@ -384,7 +386,7 @@ public:
 
 	// Used by script optimization to update lambda ingredients
 	// after compilation.
-	void SetFrameSize(size_t _frame_size) { frame_size = std::move(_frame_size); }
+	void SetFrameSize(size_t _frame_size) { frame_size = _frame_size; }
 
 private:
 	IDPtr id;
@@ -396,6 +398,8 @@ private:
 	std::set<EventGroupPtr> groups;
 	};
 
+using FunctionIngredientsPtr = std::shared_ptr<FunctionIngredients>;
+
 extern std::vector<CallInfo> call_stack;
 
 /**

src/Type.cc

@@ -699,11 +699,14 @@ TypePtr SetType::ShallowClone()
 
 SetType::~SetType() = default;
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 FuncType::Capture::Capture(detail::IDPtr _id, bool _deep_copy)
 	: id(std::move(_id)), deep_copy(_deep_copy)
 	{
 	is_managed = id ? ZVal::IsManagedType(id->GetType()) : false;
 	}
+#pragma GCC diagnostic pop
 
 FuncType::FuncType(RecordTypePtr arg_args, TypePtr arg_yield, FunctionFlavor arg_flavor)
 	: Type(TYPE_FUNC), args(std::move(arg_args)), arg_types(make_intrusive<TypeList>()),

src/Type.h

@@ -516,17 +516,31 @@ public:
 	public:
 		Capture(detail::IDPtr _id, bool _deep_copy);
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+		Capture(const Capture&) = default;
+		Capture(Capture&&) = default;
+		Capture& operator=(const Capture&) = default;
+		Capture& operator=(Capture&&) = default;
+		~Capture() = default;
+
 		auto& Id() const { return id; }
 		bool IsDeepCopy() const { return deep_copy; }
 		bool IsManaged() const { return is_managed; }
 
 		// For script optimization:
 		void SetID(detail::IDPtr new_id) { id = std::move(new_id); }
+#pragma GCC diagnostic pop
 
-	private:
-		detail::IDPtr id;
-		bool deep_copy;
-		bool is_managed;
+		[[deprecated(
+			"Remove in v7.1.  Use non-default constructor and associated accessors.")]] detail::
+			IDPtr id;
+		[[deprecated(
+			"Remove in v7.1.  Use non-default constructor and associated accessors.")]] bool
+			deep_copy;
+		[[deprecated(
+			"Remove in v7.1.  Use non-default constructor and associated accessors.")]] bool
+			is_managed;
 		};
 
 	using CaptureList = std::vector<Capture>;

src/script_opt/ScriptOpt.cc

@@ -50,14 +50,14 @@ void analyze_func(ScriptFuncPtr f)
 
 void analyze_lambda(LambdaExpr* l)
 	{
-	auto& mf = l->MasterFunc();
-	analyze_func(mf);
-	lambdas.insert(mf.get());
+	auto& pf = l->PrimaryFunc();
+	analyze_func(pf);
+	lambdas.insert(pf.get());
 	}
 
 void analyze_when_lambda(LambdaExpr* l)
 	{
-	when_lambdas.insert(l->MasterFunc().get());
+	when_lambdas.insert(l->PrimaryFunc().get());
 	}
 
 bool is_when_lambda(const ScriptFunc* f)

src/script_opt/ZAM/Expr.cc

@@ -785,9 +785,9 @@ const ZAMStmt ZAMCompiler::BuildLambda(int n_slot, LambdaExpr* le)
 	int ncaptures = captures ? captures->size() : 0;
 
 	auto aux = new ZInstAux(ncaptures);
-	aux->master_func = le->MasterFunc();
+	aux->primary_func = le->PrimaryFunc();
 	aux->lambda_name = le->Name();
-	aux->id_val = le->Ingredients()->GetID().get();
+	aux->id_val = le->Ingredients()->GetID();
 
 	for ( int i = 0; i < ncaptures; ++i )
 		{
@@ -799,7 +799,7 @@ const ZAMStmt ZAMCompiler::BuildLambda(int n_slot, LambdaExpr* le)
 			aux->Add(i, FrameSlot(id_i), id_i->GetType());
 		}
 
-	auto z = ZInstI(OP_LAMBDA_VV, n_slot, le->MasterFunc()->FrameSize());
+	auto z = ZInstI(OP_LAMBDA_VV, n_slot, le->PrimaryFunc()->FrameSize());
 	z.op_type = OP_VV_I2;
 	z.aux = aux;
 
@@ -946,7 +946,7 @@ const ZAMStmt ZAMCompiler::AssignToCall(const ExprStmt* e)
 const ZAMStmt ZAMCompiler::DoCall(const CallExpr* c, const NameExpr* n)
 	{
 	auto func = c->Func()->AsNameExpr();
-	auto func_id = func->Id();
+	auto func_id = func->IdPtr();
 	auto& args = c->Args()->Exprs();
 
 	int nargs = args.length();

src/script_opt/ZAM/Ops.in

@@ -2112,7 +2112,7 @@ assign-val v
 eval	auto v = globals[z.v2].id->GetVal();
 	if ( ! v )
 		{
-		ZAM_run_time_error(z.loc, "value used but not set", z.aux->id_val);
+		ZAM_run_time_error(z.loc, "value used but not set", z.aux->id_val.get());
 		break;
 		}
 
@@ -2180,8 +2180,8 @@ eval	flow = FLOW_BREAK;
 internal-op Lambda
 type VV
 eval	auto& aux = z.aux;
-	auto& master_func = aux->master_func;
-	auto& body = master_func->GetBodies()[0].stmts;
+	auto& primary_func = aux->primary_func;
+	auto& body = primary_func->GetBodies()[0].stmts;
 	ASSERT(body->Tag() == STMT_ZAM);
 	auto lamb = make_intrusive<ScriptFunc>(aux->id_val);
 	lamb->AddBody(body, z.v2);

src/script_opt/ZAM/Vars.cc

@@ -26,7 +26,7 @@ bool ZAMCompiler::IsUnused(const IDPtr& id, const Stmt* where) const
 
 bool ZAMCompiler::IsCapture(const ID* id) const
 	{
-	auto c = pf->CapturesOffsets();
+	const auto& c = pf->CapturesOffsets();
 	return c.find(id) != c.end();
 	}
 
@@ -75,7 +75,7 @@ const ZAMStmt ZAMCompiler::LoadGlobal(const ID* id)
 
 	// We use the id_val for reporting used-but-not-set errors.
 	z.aux = new ZInstAux(0);
-	z.aux->id_val = id;
+	z.aux->id_val = {NewRef{}, const_cast<ID*>(id)};
 
 	return AddInst(z, true);
 	}

src/script_opt/ZAM/ZInst.h

@@ -432,7 +432,7 @@ public:
 	bool* is_managed = nullptr;
 
 	// Ingredients associated with lambdas ...
-	ScriptFuncPtr master_func;
+	ScriptFuncPtr primary_func;
 
 	// ... and its name.
 	std::string lambda_name;
@@ -445,7 +445,7 @@ public:
 	std::unique_ptr<CatArg>* cat_args = nullptr;
 
 	// Used for accessing function names.
-	const ID* id_val = nullptr;
+	IDPtr id_val = nullptr;
 
 	// Whether the instruction can lead to globals/captures changing.
 	// Currently only needed by the optimizer, but convenient to