Edit pass over changes before merge.

src/Expr.cc

@@ -2639,7 +2639,7 @@ Val* IndexExpr::Eval(Frame* f) const
 			}
 		}
 	else
-	  result = Fold(v1, v2);
+		result = Fold(v1, v2);
 
 	Unref(v1);
 	Unref(v2);
@@ -2694,7 +2694,7 @@ Val* IndexExpr::Fold(Val* v1, Val* v2) const
 		break;
 
 	case TYPE_TABLE:
-	  v = v1->AsTableVal()->Lookup(v2); // Then, we jump into the TableVal here.
+		v = v1->AsTableVal()->Lookup(v2); // Then, we jump into the TableVal here.
 		break;
 
 	case TYPE_STRING:
@@ -3865,7 +3865,6 @@ FlattenExpr::FlattenExpr(Expr* arg_op)
 	SetType(tl);
 	}
 
-
 Val* FlattenExpr::Fold(Val* v) const
 	{
 	RecordVal* rv = v->AsRecordVal();
@@ -4317,11 +4316,11 @@ void CallExpr::ExprDescribe(ODesc* d) const
 		args->Describe(d);
 	}
 
-LambdaExpr::LambdaExpr(std::unique_ptr<function_ingredients> ing,
+LambdaExpr::LambdaExpr(std::unique_ptr<function_ingredients> arg_ing,
-		       std::shared_ptr<id_list> outer_ids) : Expr(EXPR_LAMBDA)
+		       std::shared_ptr<id_list> arg_outer_ids) : Expr(EXPR_LAMBDA)
 	{
-	ingredients = std::move(ing);
+	ingredients = std::move(arg_ing);
-	this->outer_ids = std::move(outer_ids);
+	outer_ids = std::move(arg_outer_ids);
 
 	SetType(ingredients->id->Type()->Ref());
 
@@ -4334,7 +4333,7 @@ LambdaExpr::LambdaExpr(std::unique_ptr<function_ingredients> ing,
 		ingredients->frame_size,
 		ingredients->priority);
 
-	dummy_func->SetOuterIDs(this->outer_ids);
+	dummy_func->SetOuterIDs(outer_ids);
 
 	// Get the body's "string" representation.
 	ODesc d;
@@ -4342,23 +4341,23 @@ LambdaExpr::LambdaExpr(std::unique_ptr<function_ingredients> ing,
 	const char* desc = d.Description();
 
 	// Install that in the global_scope
+	//
+	// TODO(robin): Let's canonify these names a bit, otherwise somebody
+	// iterating through the namespace might be up surprised ... How
+	// about calling them "lambda_<short_hash_of(desc)>"?
 	ID* id = install_ID(desc, current_module.c_str(), true, false);
 
 	// Update lamb's name
 	dummy_func->SetName(desc);
 
-	// When the id goes away it will unref v.
 	Val* v = new Val(dummy_func);
-
+	id->SetVal(v); // id will unref v when its done.
-	// id will unref v when its done.
-	id->SetVal(v);
 	id->SetType(ingredients->id->Type()->Ref());
 	id->SetConst();
 	}
 
 Val* LambdaExpr::Eval(Frame* f) const
 	{
-
 	BroFunc* lamb = new BroFunc(
 		ingredients->id,
 		ingredients->body,
@@ -4368,6 +4367,8 @@ Val* LambdaExpr::Eval(Frame* f) const
 
 	lamb->AddClosure(outer_ids, f);
 
+	// TODO(robin): Similar to above, plus: Could we cache this name?
+	// Isn't the same on every on Eval()?
 	ODesc d;
 	lamb->Describe(&d);
 	const char* desc = d.Description();
@@ -4375,7 +4376,7 @@ Val* LambdaExpr::Eval(Frame* f) const
 	// Set name to corresponding dummy func.
 	// Allows for lookups by the receiver.
 	lamb->SetName(desc);
-	
+
 	return new Val(lamb);
 	}
 
@@ -4390,7 +4391,7 @@ TraversalCode LambdaExpr::Traverse(TraversalCallback* cb) const
 	TraversalCode tc = cb->PreExpr(this);
 	HANDLE_TC_EXPR_PRE(tc);
 
-        tc = ingredients->body->Traverse(cb);
+	tc = ingredients->body->Traverse(cb);
 	HANDLE_TC_EXPR_POST(tc);
 
 	tc = cb->PostExpr(this);

src/Expr.h

@@ -12,10 +12,9 @@
 #include "Debug.h"
 #include "EventHandler.h"
 #include "TraverseTypes.h"
-#include "Func.h" // function_ingredients
 
-#include <memory> // std::shared_ptr
+#include <memory>
-#include <utility> // std::move
+#include <utility>
 
 typedef enum {
 	EXPR_ANY = -1,
@@ -661,7 +660,7 @@ protected:
 	friend class Expr;
 	IndexExpr()	{ }
 
-        Val* Fold(Val* v1, Val* v2) const override;
+	Val* Fold(Val* v1, Val* v2) const override;
 
 	void ExprDescribe(ODesc* d) const override;
 
@@ -944,7 +943,7 @@ public:
 
 protected:
 	friend class Expr;
-        CallExpr()	{ func = 0; args = 0; }
+	CallExpr()	{ func = 0; args = 0; }
 
 	void ExprDescribe(ODesc* d) const override;
 
@@ -971,6 +970,13 @@ protected:
 
 private:
 	std::unique_ptr<function_ingredients> ingredients;
+
+	// TODO(robin): I'm wondering if we should just generally pass all
+	// the new "id_list" instances by value (or const ref where it
+	// works). It's going to be very small and easy to copy; and a
+	// shared_ptr with dynamic memory comes with overhead, too. (I'm
+	// actually tempted to suggest the same for "ingredients" though
+	// that's less clear.)
 	std::shared_ptr<id_list> outer_ids;
 };
 

src/Frame.cc

@@ -3,7 +3,8 @@
 #include "zeek-config.h"
 
 #include <string>
-#include <algorithm> // std::any_of
+#include <algorithm>
+#include <broker/error.hh>
 
 #include "Frame.h"
 #include "Stmt.h"
@@ -11,7 +12,6 @@
 #include "Trigger.h"
 
 #include "broker/Data.h"
-#include <broker/error.hh>
 
 vector<Frame*> g_frame_stack;
 
@@ -50,28 +50,27 @@ Frame::Frame(const Frame* other, bool view)
 	break_before_next_stmt = false;
         break_on_return = false;
 	delayed = false;
- 
+
 	if ( is_view )
-	  {
+		frame = other->frame;
-	  frame = other->frame;
-	  }
 	else
-	  {
+		{
-	  if (trigger)
+		if  ( trigger )
-	    Ref(trigger);
+			Ref(trigger);
-	  for ( int i = 0; i < size; ++i )
+
-	    frame[i] = other->frame[i] ? other->frame[i]->Clone() : 0;
+		for ( int i = 0; i < size; ++i )
-	  }
+			frame[i] = other->frame[i] ? other->frame[i]->Clone() : 0;
+		}
 	}
 
 Frame::~Frame()
 	{
 	// Deleting a Frame that is a view is a no-op.
 	if ( ! is_view )
-	  {
+		{
-	  Unref(trigger);
+		Unref(trigger);
-	  Release();
+		Release();
-	  }
+		}
 	}
 
 void Frame::SetElement(int n, Val* v)
@@ -85,19 +84,19 @@ void Frame::SetElement(const ID* id, Val* v)
 	SetElement(id->Offset(), v);
 	}
 
-
 Val* Frame::GetElement(const ID* id) const
 	{
-	if (HasOuterIDs())
+	if ( HasOuterIDs() )
 		{
 		auto where = offset_map.find(std::string(id->Name()));
-		if (where != offset_map.end())
+		if ( where != offset_map.end() )
-			return frame[ where->second ];
+			return frame[where->second];
 		}
+
 	return frame[id->Offset()];
 	}
 
-void Frame::AddElement(ID* id, Val* v)
+void Frame::AddElement(const ID* id, Val* v)
 	{
 	this->SetElement(id, v);
 	}
@@ -152,7 +151,7 @@ Frame* Frame::Clone()
 	{
 	Frame* f = new Frame(size, function, func_args);
 	f->Clear();
-	
+
 	for ( int i = 0; i < size; ++i )
 		f->frame[i] = frame[i] ? frame[i]->Clone() : 0;
 
@@ -341,15 +340,14 @@ bool Frame::CaptureContains(const ID* i) const
 	return where != offset_map.end();
 	}
 
-ClosureFrame::ClosureFrame(Frame* closure, Frame* not_closure,
+ClosureFrame::ClosureFrame(Frame* arg_closure, Frame* not_closure,
 	std::shared_ptr<id_list> outer_ids) : Frame(not_closure, true)
 	{
-	assert(closure);
+	assert(arg_closure);
-	assert(outer_ids);
 
-	this->closure = closure;
+	closure = arg_closure;
 	body = not_closure;
-	
+
 	SetOuterIDs(outer_ids);
 	}
 
@@ -358,6 +356,9 @@ ClosureFrame::~ClosureFrame()
 	// No need to Unref the closure. BroFunc handles this.
 	// Unref body though. When the ClosureFrame is done, so is
 	// the frame that is is wrapped around.
+
+	// TODO(robin): It would be good handle body & closure the same in
+	// terms needing to ref/unref; it's easy to get confused otherwise.
 	Unref(body);
 	}
 
@@ -368,18 +369,20 @@ Val* ClosureFrame::GetElement(const ID* id) const
 		int my_offset = offset_map.at(std::string(id->Name()));
 		return ClosureFrame::GatherFromClosure(this, id, my_offset);
 		}
-	return this->NthElement(id->Offset());
+
+	return NthElement(id->Offset());
 	}
 
 void ClosureFrame::SetElement(const ID* id, Val* v)
 	{
 	if ( CaptureContains(id) )
-	  {
+		{
-	  int my_offset = offset_map.at(std::string(id->Name()));
+		int my_offset = offset_map.at(std::string(id->Name()));
-	  ClosureFrame::SetInClosure(this, id, v, my_offset);
+		ClosureFrame::SetInClosure(this, id, v, my_offset);
-	  }
+		return;
-	else
+		}
-	  this->Frame::SetElement(id->Offset(), v);
+
+	Frame::SetElement(id->Offset(), v);
 	}
 
 Frame* ClosureFrame::Clone()
@@ -397,18 +400,18 @@ Frame* ClosureFrame::SelectiveClone(id_list* choose)
 	id_list us;
 	// and
 	id_list them;
-	
-	for (const auto& we : *choose)
-	  {
-	    if ( CaptureContains(we) )
-	      us.append(we);
-	    else
-	      them.append(we);
-	  }
 
-	Frame* me = this->closure->SelectiveClone(&us);
+	for (const auto& we : *choose)
+		{
+		if ( CaptureContains(we) )
+			us.append(we);
+		else
+			them.append(we);
+		}
+
+	Frame* me = closure->SelectiveClone(&us);
 	// and
-	Frame* you = this->body->SelectiveClone(&them);
+	Frame* you = body->SelectiveClone(&them);
 
 	ClosureFrame* who = new ClosureFrame(me, you, nullptr);
 	who->offset_map = offset_map;
@@ -422,17 +425,22 @@ broker::expected<broker::data> ClosureFrame::Serialize() const
 	rval.emplace_back(std::string("ClosureFrame"));
 
 	auto om = SerializeOffsetMap();
-	if ( ! om ) return broker::ec::invalid_data;
+	if ( ! om )
+		return broker::ec::invalid_data;
+
 	rval.emplace_back( *om );
 
 	auto cl = closure->Serialize();
-	if ( ! cl ) broker::ec::invalid_data;
+	if ( ! cl )
+		return broker::ec::invalid_data;
+
 	rval.emplace_back( *cl );
 
 	auto bo = body->Serialize();
-	if ( ! bo ) broker::ec::invalid_data;
+	if ( ! bo )
-	rval.emplace_back( *bo );
+		return broker::ec::invalid_data;
 
+	rval.emplace_back(*bo);
 	return {std::move(rval)};
 	}
 
@@ -440,8 +448,8 @@ broker::expected<broker::data> Frame::SerializeOffsetMap() const
 	{
 	broker::vector rval;
 
-	std::for_each(offset_map.begin(), offset_map.end(), 
+	std::for_each(offset_map.begin(), offset_map.end(),
-		[&rval] (const std::pair<std::string, int>& e) 
+		[&rval] (const std::pair<std::string, int>& e)
 			{ rval.emplace_back(e.first); rval.emplace_back(e.second);});
 
 	return {std::move(rval)};
@@ -449,34 +457,32 @@ broker::expected<broker::data> Frame::SerializeOffsetMap() const
 
 bool ClosureFrame::UnserializeIntoOffsetMap(const broker::vector& data, std::unordered_map<std::string, int>& target)
 	{
-	#define GET_OR_RETURN(type, name, index)							\
-		if (auto __##name##__ = broker::get_if<type>(data[index]))		\
-			name = *__##name##__;										\
-		else															\
-			return false;												\
-
 	assert(target.size() == 0);
 
 	std::unordered_map<std::string, int> rval;
 
 	for (broker::vector::size_type i = 0; i < data.size(); i += 2)
 		{
-		std::string key;
+		auto key = broker::get_if<std::string>(data[i]);
-		int offset;
+		if ( ! key )
-		GET_OR_RETURN(std::string, key, i)
+			return false;
-		GET_OR_RETURN(broker::integer, offset, i+1)
+
-		target.insert( {std::move(key), std::move(offset)} );
+		auto offset = broker::get_if<broker::integer>(data[i+1]);
+		if ( ! offset )
+			return false;
+
+		target.insert( {std::move(*key), std::move(*offset)} );
 		}
 
 	return true;
-	#undef GET_OR_RETURN
 	}
 
-// Each ClosureFrame knows all of the outer IDs that are used inside of it. This is known at
+// Each ClosureFrame knows all of the outer IDs that are used inside of it.
-// parse time. These leverage that. If frame_1 encloses frame_2 then the location of a lookup
+// This is known at parse time. These leverage that. If frame_1 encloses
-// for an outer id in frame_2 can be determined by checking if that id is also an outer id in
+// frame_2 then the location of a lookup for an outer id in frame_2 can be
-// frame_2. If it is not, then frame_2 owns the id and the lookup is done there, otherwise, 
+// determined by checking if that id is also an outer id in frame_2. If it is
-// go deeper.
+// not, then frame_2 owns the id and the lookup is done there, otherwise, go
+// deeper.
 Val* ClosureFrame::GatherFromClosure(const Frame* start, const ID* id, const int offset)
 	{
 	const ClosureFrame* conductor = dynamic_cast<const ClosureFrame*>(start);
@@ -485,13 +491,13 @@ Val* ClosureFrame::GatherFromClosure(const Frame* start, const ID* id, const int
 	// was born. We differ to its maping as it is older and wiser. Otherwise, we use our own.
 	if ( ! conductor )
 		{
-		if (start->HasOuterIDs())
+		if ( start->HasOuterIDs() )
 			return start->GetElement(id);
 
 		return start->NthElement(offset);
 		}
 
-	if (conductor->CaptureContains(id))
+	if ( conductor->CaptureContains(id) )
 		return ClosureFrame::GatherFromClosure(conductor->closure, id, offset);
 
 	return conductor->NthElement(offset);
@@ -502,11 +508,11 @@ void ClosureFrame::SetInClosure(Frame* start, const ID* id, Val* val, const int
 	ClosureFrame* conductor = dynamic_cast<ClosureFrame*>(start);
 
 	if ( ! conductor )
-	  start->SetElement(offset, val);
+		start->SetElement(offset, val);
 
-	else if (conductor->CaptureContains(id))
+	else if ( conductor->CaptureContains(id) )
-	  ClosureFrame::SetInClosure(conductor->closure, id, val, offset);
+		ClosureFrame::SetInClosure(conductor->closure, id, val, offset);
 
 	else
-	  conductor->Frame::SetElement(offset, val);
+		conductor->Frame::SetElement(offset, val);
 	}

src/Frame.h

@@ -3,11 +3,11 @@
 #ifndef frame_h
 #define frame_h
 
-#include <vector>
+#include <memory>
-#include <unordered_map>
 #include <string>
-#include <memory> // std::shared_ptr
+#include <unordered_map>
-#include <utility> // std::pair
+#include <utility>
+#include <vector>
 
 #include <broker/data.hh>
 #include <broker/expected.hh>
@@ -19,21 +19,26 @@ class Trigger;
 class CallExpr;
 class Val;
 
+// TODO(robin): As discussed, I think this would benefit from merging the two closes.
+// I don't think having that one derived class buys us much in the end in terms
+// of performance, and it makes the code quite a bit more complex.
+
 class Frame : public BroObj {
-friend class BroFunc;
 public:
 	Frame(int size, const BroFunc* func, const val_list *fn_args);
-	// Constructs a copy or view of other. If a view is constructed the
+
-	// destructor will not change other's state on deletion.
+	// Constructs a copy, or view, of another frame. If a view is
+	// constructed the destructor will not change other's state on
+	// deletion.
 	Frame(const Frame* other, bool is_view = false);
+
 	~Frame() override;
 
-	Val* NthElement(int n) const { return frame[n]; }
+	Val* NthElement(int n) const	{ return frame[n]; }
 	void SetElement(int n, Val* v);
 	virtual void SetElement(const ID* id, Val* v);
-
 	virtual Val* GetElement(const ID* id) const;
-	void AddElement(ID* id, Val* v);
+	void AddElement(const ID* id, Val* v);
 
 	void Reset(int startIdx);
 	void Release();
@@ -44,6 +49,9 @@ public:
 	const BroFunc* GetFunction() const	{ return function; }
 	const val_list* GetFuncArgs() const	{ return func_args; }
 
+ 	// Changes the function associated with the frame.
+	void SetFunction(BroFunc* func)	{ function = func; }
+
 	// Next statement to be executed in the context of this frame.
 	void SetNextStmt(Stmt* stmt)	{ next_stmt = stmt; }
 	Stmt* GetNextStmt() const	{ return next_stmt; }
@@ -83,23 +91,25 @@ public:
 	/**
 	 * Instantiates a Frame from a serialized one.
 	 *
-	 * @return a pair. the first item is the status of the serialization,
+	 * @return a pair in which the first item is the status of the serialization;
-	 * the second is the Unserialized frame with reference count +1
+	 * and the second is the unserialized frame with reference count +1, or
+	 * null if the serialization wasn't succesful.
 	 */
 	static std::pair<bool, Frame*> Unserialize(const broker::vector& data);
 
 	/**
-	 * Installs *outer_ids* in this Frame's offset_map. 
+	 * Installs *outer_ids* into the set of IDs the frame knows offsets for.
-	 * 
+	 *
-	 * Note: This needs to be done before serializing a Frame to guarantee that 
+	 * Note: This needs to be done before serializing a Frame to guarantee that
 	 * the unserialized frame will perform lookups properly.
-	 * 
+	 *
 	 * @param outer_ids the ids that this frame holds
 	 */
 	void SetOuterIDs(std::shared_ptr<id_list> outer_ids);
 
 	/**
-	 * @return does this frame have an initialized offset_map?
+	 * @return does this frame have any IDs installed to know their
+	 * offsets?
 	 */
 	bool HasOuterIDs() const { return offset_map.size(); }
 
@@ -129,7 +139,7 @@ protected:
 
 	/**
 	 * Serializes this Frame's offset map.
-	 * 
+	 *
 	 * @return a serialized version of the offset map.
 	 */
 	broker::expected<broker::data> SerializeOffsetMap() const;
@@ -148,11 +158,11 @@ protected:
 	const CallExpr* call;
 	bool delayed;
 
-	/** 
+	/**
 	 * Maps ID names to the offsets they had when passed into the frame.
-	 * 
+	 *
 	 * A frame that has been serialized maintains its own map between IDs and
-	 * their offsets. This is because a serialized frame is not guaranteed to 
+	 * their offsets. This is because a serialized frame is not guaranteed to
 	 * be unserialized somewhere where the offsets for the IDs that it contains
 	 * are the same.
 	 */
@@ -160,8 +170,8 @@ protected:
 
 private:
 
-	/** 
+	/**
-	 * Rather or not this frame is a view of another one. Frames that
+	 * Wether or not this frame is a view of another one. Frames that
 	 * are views do not delete their underlying frame on deletion.
 	 */
 	bool is_view;
@@ -169,16 +179,17 @@ private:
 
 
 /**
- * Class that allows for actions in both a regular frame and a closure frame
+ * Frame variant that allows for performing operation on both a regular frame
- * according to a list of outer IDs captured in the closure passed into the
+ * and an additional closure frame according to a list of outer IDs captured
- * constructor. 
+ * in the closure passed into the constructor.
  */
 class ClosureFrame : public Frame {
 public:
 	/**
-	 * Constructs a closure Frame from a closure and body frame, and a list of ids
+	 * Constructs a closure frame from a closure and body frame, and a
-	 * that this frame should refer to its closure to for values. For non closure
+	 * list of ids for which this frame should refer to its closure for
-	 * related operations the ClosureFrame is just a view of the body frame.
+	 * values. For other, non-closure ID the ClosureFrame is just a view
+	 * of the body frame. 
 	 *
 	 * @param closure the frame that holds IDs in *outer_ids*.
 	 * @param body the frame to refer to for all non-closure actions.
@@ -199,15 +210,14 @@ public:
 		(const broker::vector& data, std::unordered_map<std::string, int>& target);
 
 private:
-
 	/**
-	 * Finds the Value corresponding to *id* in the closure of *start*.
+	 * Finds the value corresponding to *id* in the closure of *start*.
-	 * 
+	 *
 	 * @param start the frame to begin the search from
 	 * @param id the ID whose corresponding value is to be collected.
 	 * @param offset the offset at which to look for id's value when its
 	 * frame has been found.
-	 * @return the Value corresponding to *id*.
+	 * @return the value corresponding to *id*.
 	 */
 	static Val* GatherFromClosure(const Frame* start, const ID* id, const int offset);
 
@@ -215,7 +225,7 @@ private:
 	 * Sets the Value corresponding to *id* in the closure of *start* to *val*
 	 *
 	 * @param start the frame to begin the search from
-	 * @param val the Value to associate with *id* in the closure.
+	 * @param val the value to associate with *id* in the closure.
 	 * @param id the ID whose corresponding value is to be updated.
 	 * @param offset the offset at which to look for id's value when its
 	 * frame has been found.

src/Func.cc

@@ -28,6 +28,7 @@
 #include <signal.h>
 
 #include <algorithm>
+
 #include <broker/error.hh>
 
 #include "Base64.h"
@@ -127,7 +128,6 @@ void Func::AddBody(Stmt* /* new_body */, id_list* /* new_inits */,
 	Internal("Func::AddBody called");
 	}
 
-
 Func* Func::DoClone()
 	{
 	// By default, ok just to return a reference. Func does not have any state
@@ -267,7 +267,7 @@ std::pair<bool, Val*> Func::HandlePluginResult(std::pair<bool, Val*> plugin_resu
 	}
 
 BroFunc::BroFunc(ID* arg_id, Stmt* arg_body, id_list* aggr_inits,
-		int arg_frame_size, int priority) : Func(BRO_FUNC)
+		 int arg_frame_size, int priority) : Func(BRO_FUNC)
 	{
 	name = arg_id->Name();
 	type = arg_id->Type()->Ref();
@@ -284,7 +284,7 @@ BroFunc::BroFunc(ID* arg_id, Stmt* arg_body, id_list* aggr_inits,
 
 BroFunc::~BroFunc()
 	{
-	std::for_each(bodies.begin(), bodies.end(), 
+	std::for_each(bodies.begin(), bodies.end(),
 		[](Body& b) { Unref(b.stmts); });
 	Unref(closure);
 	}
@@ -327,10 +327,9 @@ Val* BroFunc::Call(val_list* args, Frame* parent) const
 		}
 
 	Frame* f = new Frame(frame_size, this, args);
+
 	if ( closure )
-		{
+		f = new ClosureFrame(closure, f, outer_ids);
-		f = new ClosureFrame(this->closure, f, this->outer_ids);
-		}
 
 	// Hand down any trigger.
 	if ( parent )
@@ -389,6 +388,7 @@ Val* BroFunc::Call(val_list* args, Frame* parent) const
 			if ( Flavor() == FUNC_FLAVOR_FUNCTION )
 				{
 				Unref(f);
+				// TODO(robin): Shouldn't "result" have been left unset in this case?
 				Unref(result);
 				throw;
 				}
@@ -485,15 +485,15 @@ void BroFunc::AddBody(Stmt* new_body, id_list* new_inits, int new_frame_size,
 
 void BroFunc::AddClosure(std::shared_ptr<id_list> ids, Frame* f)
 	{
-	this->SetOuterIDs(ids);
+	SetOuterIDs(ids);
-	this->SetClosureFrame(f);
+	SetClosureFrame(f);
 	}
 
 void BroFunc::SetClosureFrame(Frame* f)
 	{
 	if ( closure )
-	  reporter->InternalError
+		reporter->InternalError("Tried to override closure for BroFunc %s.",
-	    ("Tried to override closure for BroFunc %s.", this->Name());
+					Name());
 
 	closure = f ? f->SelectiveClone( outer_ids.get() ) : nullptr;
 	}
@@ -501,19 +501,22 @@ void BroFunc::SetClosureFrame(Frame* f)
 bool BroFunc::UpdateClosure(const broker::vector& data)
 	{
 	auto result = Frame::Unserialize(data);
-	if ( ! result.first ) return false;
+	if ( ! result.first )
+		return false;
 
 	Frame* new_closure = result.second;
+	if ( new_closure )
+		new_closure->SetFunction(this);
 
-	if (new_closure)
+	if ( closure )
-		new_closure->function = this;
+		Unref(closure);
 
-	if (closure) Unref(closure);
 	closure = new_closure;
 
 	return true;
 	}
 
+
 Func* BroFunc::DoClone()
 	{
 	// A BroFunc could hold a closure. In this case a clone of it must
@@ -523,7 +526,6 @@ Func* BroFunc::DoClone()
 	CopyStateInto(other);
 
 	other->frame_size = frame_size;
-
 	other->closure = closure ? closure->Clone() : nullptr;
 	other->outer_ids = outer_ids;
 
@@ -532,16 +534,15 @@ Func* BroFunc::DoClone()
 
 broker::expected<broker::data> BroFunc::SerializeClosure() const
 	{
-	if (! closure) 
+	if ( ! closure )
-		{
 		return broker::vector({"Frame"});
-		}
 
 	closure->SetOuterIDs(outer_ids);
 
 	auto e = closure->Serialize();
+	if ( ! e )
+		return broker::ec::invalid_data;
 
-	if ( !e ) return broker::ec::invalid_data;
 	return *e;
 	}
 

src/Func.h

@@ -4,7 +4,7 @@
 #define func_h
 
 #include <utility>
-#include <memory> // std::shared_ptr, std::unique_ptr
+#include <memory>
 
 #include <broker/data.hh>
 #include <broker/expected.hh>
@@ -74,7 +74,8 @@ public:
 
 protected:
 	Func();
-	// Copies this functions state into other.
+
+	// Copies this function's state into other.
 	void CopyStateInto(Func* other) const;
 
 	// Helper function for handling result of plugin hook.
@@ -96,13 +97,12 @@ public:
 	~BroFunc() override;
 
 	int IsPure() const override;
-
 	Val* Call(val_list* args, Frame* parent) const override;
 
 	/**
-	 * Adds adds a closure to the function. Closures are cloned and 
+	 * Adds adds a closure to the function. Closures are cloned and
-	 * future calls to BroFunc will not modify *f*
+	 * future calls to BroFunc methods will not modify *f*.
-	 * 
+	 *
 	 * @param ids IDs that are captured by the closure.
 	 * @param f the closure to be captured.
 	 */
@@ -110,26 +110,21 @@ public:
 
 	/**
 	 * Replaces the current closure with one built from *data*
-	 * 
+	 *
 	 * @param data a serialized closure
 	 */
 	bool UpdateClosure(const broker::vector& data);
 
-	void AddBody(Stmt* new_body, id_list* new_inits, int new_frame_size,
-			int priority) override;
-
-	/**
-	 * Clones this function along with its closures.
-	 */
-	Func* DoClone() override;
-
 	/**
 	 * Serializes this function's closure.
-	 * 
+	 *
 	 * @return a serialized version of the function's closure.
 	 */
 	broker::expected<broker::data> SerializeClosure() const;
 
+	void AddBody(Stmt* new_body, id_list* new_inits, int new_frame_size,
+			int priority) override;
+
 	/** Sets this function's outer_id list. */
 	void SetOuterIDs(std::shared_ptr<id_list> ids)
 		{ outer_ids = std::move(ids); }
@@ -140,17 +135,22 @@ protected:
 	BroFunc() : Func(BRO_FUNC)	{}
 	Stmt* AddInits(Stmt* body, id_list* inits);
 
-	int frame_size;
+	/**
+	 * Clones this function along with its closures.
+	 */
+	Func* DoClone() override;
 
-private:
 	/**
 	 * Performs a selective clone of *f* using the IDs that were
 	 * captured in the function's closure.
-	 * 
+	 *
 	 * @param f the frame to be cloned.
 	 */
 	void SetClosureFrame(Frame* f);
 
+private:
+	int frame_size;
+
 	// List of the outer IDs used in the function.
 	std::shared_ptr<id_list> outer_ids = nullptr;
 	// The frame the BroFunc was initialized in.
@@ -190,16 +190,15 @@ struct CallInfo {
 	const val_list* args;
 };
 
-// Struct that collects the arguments for a Func.
+// Struct that collects all the specifics defining a Func. Used for BroFuncs
-// Used for BroFuncs with closures.
+// with closures.
-struct function_ingredients
+struct function_ingredients {
-	{
+	ID* id;
-		ID* id;
+	Stmt* body;
-		Stmt* body;
+	id_list* inits;
-		id_list* inits;
+	int frame_size;
-		int frame_size;
+	int priority;
-		int priority;
+	Scope* scope;
-		Scope* scope;
 	};
 
 extern vector<CallInfo> call_stack;

src/Scope.cc

@@ -171,7 +171,7 @@ ID* install_ID(const char* name, const char* module_name,
 	if ( is_export || ! module_name ||
 	     (is_global &&
 	      normalized_module_name(module_name) == GLOBAL_MODULE_NAME) )
-		    scope = SCOPE_GLOBAL;
+		scope = SCOPE_GLOBAL;
 	else if ( is_global )
 		scope = SCOPE_MODULE;
 	else
@@ -188,7 +188,7 @@ ID* install_ID(const char* name, const char* module_name,
 		id->SetOffset(top_scope->Length());
 		top_scope->Insert(full_name, id);
 		}
-		
+
 	return id;
 	}
 

src/Val.cc

@@ -115,7 +115,8 @@ Val* Val::DoClone(CloneState* state)
 		// Derived classes are responsible for this. Exception:
 		// Functions and files. There aren't any derived classes.
 		if ( type->Tag() == TYPE_FUNC )
-		        return new Val(this->AsFunc()->DoClone());
+		        return new Val(AsFunc()->DoClone());
+
 		if ( type->Tag() == TYPE_FILE )
 			{
 			// I think we can just ref the file here - it is unclear what else
@@ -1734,7 +1735,7 @@ Val* TableVal::Default(Val* index)
 			}
 
 		else
-		        def_val = def_attr->AttrExpr()->Eval(0);
+			def_val = def_attr->AttrExpr()->Eval(0);
 		}
 
 	if ( ! def_val )
@@ -2144,23 +2145,23 @@ int TableVal::CheckAndAssign(Val* index, Val* new_val)
 	}
 
 void TableVal::InitDefaultFunc(Frame* f)
-        {
+	{
 	// Value aready initialized.
 	if ( def_val )
-	  return;
+		return;
 
 	Attr* def_attr = FindAttr(ATTR_DEFAULT);
 	if ( ! def_attr )
-	  return;
+		return;
 
 	BroType* ytype = Type()->YieldType();
 	BroType* dtype = def_attr->AttrExpr()->Type();
 
 	if ( dtype->Tag() == TYPE_RECORD && ytype->Tag() == TYPE_RECORD &&
-		     ! same_type(dtype, ytype) &&
+	     ! same_type(dtype, ytype) &&
-		     record_promotion_compatible(dtype->AsRecordType(),
+	     record_promotion_compatible(dtype->AsRecordType(),
-						 ytype->AsRecordType()) )
+					 ytype->AsRecordType()) )
-	  return; // TableVal::Default will handle this.
+		return; // TableVal::Default will handle this.
 
 	def_val = def_attr->AttrExpr()->Eval(f);
 	}

src/Val.h

@@ -880,8 +880,8 @@ public:
 	void InitTimer(double delay);
 	void DoExpire(double t);
 
-        // If default attribute is not a function, or the functon has
+        // If the &default attribute is not a function, or the functon has
-        // already been initialized this does nothing. Otherwise, evaluates
+        // already been initialized, this does nothing. Otherwise, evaluates
         // the function in the frame allowing it to capture its closure.
         void InitDefaultFunc(Frame* f);
 

src/Var.cc

@@ -438,46 +438,43 @@ TraversalCode OuterIDBindingFinder::PreExpr(const Expr* expr)
 
 // Gets a function's priority from its Scope's attributes. Errors if it sees any
 // problems.
-int get_func_priotity(attr_list* attrs)
+static int get_func_priotity(const attr_list& attrs)
 	{
 	int priority = 0;
-	if ( attrs )
+
+	for ( const auto& a : attrs )
 		{
-		for ( const auto& a : *attrs )
+		if ( a->Tag() == ATTR_DEPRECATED )
+			continue;
+
+		if ( a->Tag() != ATTR_PRIORITY )
 			{
-			if ( a->Tag() == ATTR_DEPRECATED )
+			a->Error("illegal attribute for function body");
-				continue;
+			continue;
-
-			if ( a->Tag() != ATTR_PRIORITY )
-				{
-				a->Error("illegal attribute for function body");
-				continue;
-				}
-
-			Val* v = a->AttrExpr()->Eval(0);
-			if ( ! v )
-				{
-				a->Error("cannot evaluate attribute expression");
-				continue;
-				}
-
-			if ( ! IsIntegral(v->Type()->Tag()) )
-				{
-				a->Error("expression is not of integral type");
-				continue;
-				}
-
-			priority = v->InternalInt();
 			}
+
+		Val* v = a->AttrExpr()->Eval(0);
+		if ( ! v )
+			{
+			a->Error("cannot evaluate attribute expression");
+			continue;
+			}
+
+		if ( ! IsIntegral(v->Type()->Tag()) )
+			{
+			a->Error("expression is not of integral type");
+			continue;
+			}
+
+		priority = v->InternalInt();
 		}
-		return priority;
+
+	return priority;
 	}
 
 void end_func(Stmt* body)
 	{
-	std::unique_ptr<function_ingredients> ingredients = gather_function_ingredients(body);
+	std::unique_ptr<function_ingredients> ingredients = gather_function_ingredients(pop_scope(), body);
-	
-	pop_scope();
 
 	if ( streq(ingredients->id->Name(), "anonymous-function") )
 		{
@@ -503,7 +500,7 @@ void end_func(Stmt* body)
 			ingredients->inits,
 			ingredients->frame_size,
 			ingredients->priority);
-		
+
 		ingredients->id->SetVal(new Val(f));
 		ingredients->id->SetConst();
 		}
@@ -511,21 +508,19 @@ void end_func(Stmt* body)
 	ingredients->id->ID_Val()->AsFunc()->SetScope(ingredients->scope);
 	}
 
-// Gathers all of the information from the current scope needed to build a
+std::unique_ptr<function_ingredients> gather_function_ingredients(Scope* scope, Stmt* body)
-// function and collects it into a function_ingredients struct.
-std::unique_ptr<function_ingredients> gather_function_ingredients(Stmt* body)
 	{
-	std::unique_ptr<function_ingredients> ingredients = build_unique<function_ingredients>();
+	auto ingredients = build_unique<function_ingredients>();
 
-	ingredients->frame_size = current_scope()->Length();
+	ingredients->frame_size = scope->Length();
-	ingredients->inits = current_scope()->GetInits();
+	ingredients->inits = scope->GetInits();
 
-	ingredients->scope = current_scope();
+	ingredients->scope = scope;
-	ingredients->id = ingredients->scope->ScopeID();
+	ingredients->id = scope->ScopeID();
 
-	auto attrs = ingredients->scope->Attrs();
+	auto attrs = scope->Attrs();
 
-	ingredients->priority = get_func_priotity(attrs);
+	ingredients->priority = (attrs ? get_func_priotity(*attrs) : 0);
 	ingredients->body = body;
 
 	return ingredients;

src/Var.h

@@ -29,6 +29,14 @@ extern void end_func(Stmt* body);
 extern std::unique_ptr<function_ingredients> gather_function_ingredients(Stmt* body);
 extern std::shared_ptr<id_list> gather_outer_ids(Scope* scope, Stmt* body);
 
+// Gathers all of the information from a scope and a function body needed to
+// build a function and collects it into a function_ingredients struct.
+// Gathered elements are not refeed.
+extern std::unique_ptr<function_ingredients> gather_function_ingredients(Scope* scope, Stmt* body);
+
+// Gather all IDs referenced inside a body that aren't part of a given scope.
+extern std::shared_ptr<id_list> gather_outer_ids(Scope* scope, Stmt* body);
+
 extern Val* internal_val(const char* name);
 extern Val* internal_const_val(const char* name); // internal error if not const
 extern Val* opt_internal_val(const char* name);	// returns nil if not defined

src/broker/Data.cc

@@ -342,54 +342,45 @@ struct val_converter {
 
 			return rval;
 			}
-		else if (type->Tag() == TYPE_FUNC)
+		else if ( type->Tag() == TYPE_FUNC )
 			{
-			#define GET_OR_RETURN(type, name, index)							\
+			if ( a.size() < 1 || a.size() > 2 )
-				if (auto __##name##__ = broker::get_if<type>(a[index]))			\
-					name = *__##name##__;										\
-				else															\
-					return nullptr;												\
-
-			if (a.size() > 2)
 				return nullptr;
-			
-			std::string name;
-			GET_OR_RETURN(std::string, name, 0);
 
-			auto id = global_scope()->Lookup(name.c_str());
+			auto name = broker::get_if<std::string>(a[0]);
+			if ( ! name )
+				return nullptr;
 
+			auto id = global_scope()->Lookup(name->c_str());
 			if ( ! id )
 				return nullptr;
 
 			auto rval = id->ID_Val();
-
 			if ( ! rval )
 				return nullptr;
 
 			auto t = rval->Type();
-
 			if ( ! t )
 				return nullptr;
 
-
 			if ( t->Tag() != TYPE_FUNC )
 				return nullptr;
 
-			if (a.size() == 2) // We have a closure.
+			if ( a.size() == 2 ) // We have a closure.
 				{
-				broker::vector frame;
+				auto frame = broker::get_if<broker::vector>(a[1]);
-				GET_OR_RETURN(broker::vector, frame, 1);
+				if ( ! frame )
+					return nullptr;
 
-				bool status = false;
+				auto b = dynamic_cast<BroFunc*>(rval->AsFunc());
-				if ( BroFunc* b = dynamic_cast<BroFunc*>(rval->AsFunc()))
+				if ( ! b )
-					{
+					return nullptr;
-					status = b->UpdateClosure(frame);
+
-					}
+				if ( ! b->UpdateClosure(*frame) )
-				if ( ! status ) return nullptr;
+					return nullptr;
 				}
 
 			return rval->Ref();
-			#undef GET_OR_RETURN
 			}
 		else if ( type->Tag() == TYPE_RECORD )
 			{
@@ -705,30 +696,22 @@ struct type_checker {
 			}
 		else if ( type->Tag() == TYPE_FUNC )
 			{
-			#define GET_OR_RETURN(type, name, index)							\
+			if ( a.size() < 1 || a.size() > 2 )
-				if (auto __##name##__ = broker::get_if<type>(a[index]))			\
-					name = *__##name##__;										\
-				else															\
-					return false;												\
-
-			if (a.size() > 2)
 				return false;
-			
-			std::string name;
-			GET_OR_RETURN(std::string, name, 0);
 
-			auto id = global_scope()->Lookup(name.c_str());
+			auto name = broker::get_if<std::string>(a[0]);
+			if ( ! name )
+				return false;
 
+			auto id = global_scope()->Lookup(name->c_str());
 			if ( ! id )
 				return false;
 
 			auto rval = id->ID_Val();
-
 			if ( ! rval )
 				return false;
 
 			auto t = rval->Type();
-
 			if ( ! t )
 				return false;
 
@@ -881,30 +864,32 @@ broker::expected<broker::data> bro_broker::val_to_data(Val* v)
 		return {string(v->AsFile()->Name())};
 	case TYPE_FUNC:
 	        {
-			Func* f = v->AsFunc();
+		Func* f = v->AsFunc();
-			std::string name(f->Name());
+		std::string name(f->Name());
-			broker::vector rval;
-			rval.push_back(name);
 
-			auto starts_with = [](const std::string& s ,const std::string& in) -> bool 
+		broker::vector rval;
-				{ return (0 == s.find(in)); };
+		rval.push_back(name);
 
-			if ( starts_with(name, "anonymous-function") )
+		if ( name.find("anonymous-function") == 0 )
+			{
+			// Only BroFuncs have closures.
+			if ( auto b = dynamic_cast<BroFunc*>(f) )
 				{
-				// Only BroFuncs have closures.
+				auto bc = dynamic_cast<BroFunc*>(f)->SerializeClosure();
-				if (BroFunc* b = dynamic_cast<BroFunc*>(f))
+				if ( ! bc )
-					{
-					auto bc = dynamic_cast<BroFunc*>(f)->SerializeClosure();
-					if ( ! bc )
-						return broker::ec::invalid_data;
-					rval.emplace_back(std::move(*bc));
-					}
-				else
 					return broker::ec::invalid_data;
-				}
 
-			return {std::move(rval)};
+				rval.emplace_back(std::move(*bc));
+				}
+			else
+				{
+				reporter->InternalWarning("closure with non-BroFunc");
+				return broker::ec::invalid_data;
+				}
 			}
+
+		return {std::move(rval)};
+		}
 	case TYPE_TABLE:
 		{
 		auto is_set = v->Type()->IsSet();
@@ -1058,10 +1043,10 @@ RecordVal* bro_broker::make_data_val(Val* v)
 	auto rval = new RecordVal(BifType::Record::Broker::Data);
 	auto data = val_to_data(v);
 
-	if  ( ! data ) reporter->Warning("Didn't get a value from val_to_data");
+	if  ( data )
-
-	if ( data )
 		rval->Assign(0, new DataVal(move(*data)));
+	else
+		reporter->Warning("did not get a value from val_to_data");
 
 	return rval;
 	}

src/broker/Manager.cc

@@ -1042,20 +1042,20 @@ void Manager::ProcessEvent(const broker::topic& topic, broker::zeek::Event ev)
 			vl.append(val);
 		else
 			{
-			const char* expected_name = type_name(expected_type->Tag());
+			auto expected_name = type_name(expected_type->Tag());
 
 			reporter->Warning("failed to convert remote event '%s' arg #%d,"
 						" got %s, expected %s",
 						name.data(), i, got_type,
 						expected_name);
 
-			// If we got a vector and expected a function this is possibly because of a mismatch
+			// If we got a vector and expected a function this is
-			// between anonymous-function bodies.
+			// possibly because of a mismatch between
-			if ( (strcmp( expected_name, "func") == 0) && (strcmp("vector", got_type) ==  0) )
+			// anonymous-function bodies.
-				{
+			if ( strcmp(expected_name, "func") == 0 && strcmp("vector", got_type) == 0 )
 				reporter->Warning("when sending functions the receiver must have access to a"
-				" version of that function.\nFor anonymous functions, that function must have the same body.");
+						  " version of that function.\nFor anonymous functions, that function must have the same body.");
-				}
+
 			break;
 			}
 		}

src/parse.y

@@ -1214,7 +1214,7 @@ func_body:
 
 anonymous_function:
 		TOK_FUNCTION begin_func
-		
+
 		'{'
 			{
 			saved_in_init.push_back(in_init);
@@ -1222,7 +1222,6 @@ anonymous_function:
 			}
 
 		stmt_list
-
 			{
 			in_init = saved_in_init.back();
 			saved_in_init.pop_back();
@@ -1230,8 +1229,11 @@ anonymous_function:
 
 		'}'
 			{
+                        // TODO(robin): Can move the following into end_func()?
+			// If so, we could reuse the "func_body" rule here and avoid the code duplication.
+
 			// Gather the ingredients for a BroFunc from the current scope
-			std::unique_ptr<function_ingredients> ingredients = gather_function_ingredients($5);
+			std::unique_ptr<function_ingredients> ingredients = gather_function_ingredients(current_scope(), $5);
 			std::shared_ptr<id_list> outer_ids = gather_outer_ids(pop_scope(), $5);
 
 			$$ = new LambdaExpr(std::move(ingredients), std::move(outer_ids));