Simplify NamespaceValue class hierarchy to one struct without member functions

This commit removes EmbeddedNamespaceValue and ConstEmbeddedNamespaceValue and
reduces NamespaceValue down to a simple struct without inheritance or member
functions. The code from these clases is inlined into the Namespace class. The
class hierarchy determining whether a value is const is moved to an attribute
of NamespaceValue.

This is done in preparation for changes to the locking in the Namespace class.
Currently, it relies on a recursive mutex. In the future, a shared mutex
(read/write lock) should be used instead, which cannot allow recursive locking
(without failing or risk deadlocking on lock upgrades). With this change, all
operations requiring a lock for one operation are within one function, no
recursive locking is not needed any more.

lib/base/function.hpp

@@ -60,28 +60,28 @@ private:
 	INITIALIZE_ONCE_WITH_PRIORITY([]() { \
 		Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), false); \
 		Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \
-		nsp->SetAttribute(#name, new ConstEmbeddedNamespaceValue(sf)); \
+		nsp->Set(#name, sf, true, true); \
 	}, InitializePriority::RegisterFunctions)
 
 #define REGISTER_SAFE_FUNCTION(ns, name, callback, args) \
 	INITIALIZE_ONCE_WITH_PRIORITY([]() { \
 		Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), true); \
 		Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \
-		nsp->SetAttribute(#name, new ConstEmbeddedNamespaceValue(sf)); \
+		nsp->Set(#name, sf, true, true); \
 	}, InitializePriority::RegisterFunctions)
 
 #define REGISTER_FUNCTION_NONCONST(ns, name, callback, args) \
 	INITIALIZE_ONCE_WITH_PRIORITY([]() { \
 		Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), false); \
 		Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \
-		nsp->SetAttribute(#name, new EmbeddedNamespaceValue(sf)); \
+		nsp->Set(#name, sf, false, true); \
 	}, InitializePriority::RegisterFunctions)
 
 #define REGISTER_SAFE_FUNCTION_NONCONST(ns, name, callback, args) \
 	INITIALIZE_ONCE_WITH_PRIORITY([]() { \
 		Function::Ptr sf = new icinga::Function(#ns "#" #name, callback, String(args).Split(":"), true); \
 		Namespace::Ptr nsp = ScriptGlobal::Get(#ns); \
-		nsp->SetAttribute(#name, new EmbeddedNamespaceValue(sf)); \
+		nsp->SetAttribute(#name, sf, false, true); \
 	}, InitializePriority::RegisterFunctions)
 
 }

lib/base/json-script.cpp

@@ -24,5 +24,5 @@ INITIALIZE_ONCE([]() {
 	jsonNS->Freeze();
 
 	Namespace::Ptr systemNS = ScriptGlobal::Get("System");
-	systemNS->SetAttribute("Json", new ConstEmbeddedNamespaceValue(jsonNS));
+	systemNS->Set("Json", jsonNS, true, true);
 });

lib/base/json.cpp

@@ -95,7 +95,7 @@ void EncodeNamespace(JsonEncoder<prettyPrint>& stateMachine, const Namespace::Pt
 	ObjectLock olock(ns);
 	for (const Namespace::Pair& kv : ns) {
 		stateMachine.Key(Utility::ValidateUTF8(kv.first));
-		Encode(stateMachine, kv.second->Get());
+		Encode(stateMachine, kv.second.Val);
 	}
 
 	stateMachine.EndObject();

lib/base/math-script.cpp

@@ -180,5 +180,5 @@ INITIALIZE_ONCE([]() {
 	mathNS->Freeze();
 
 	Namespace::Ptr systemNS = ScriptGlobal::Get("System");
-	systemNS->SetAttribute("Math", new ConstEmbeddedNamespaceValue(mathNS));
+	systemNS->Set("Math", mathNS, true, true);
 });

lib/base/namespace-script.cpp

@@ -63,7 +63,7 @@ static Array::Ptr NamespaceValues()
 	ArrayData values;
 	ObjectLock olock(self);
 	for (const Namespace::Pair& kv : self) {
-		values.push_back(kv.second->Get());
+		values.push_back(kv.second.Val);
 	}
 	return new Array(std::move(values));
 }

lib/base/namespace.cpp

@@ -28,7 +28,7 @@ Value Namespace::Get(const String& field) const
 	ObjectLock olock(this);
 
 	Value value;
-	if (!GetOwnField(field, &value))
+	if (!Get(field, &value))
 		BOOST_THROW_EXCEPTION(ScriptError("Namespace does not contain field '" + field + "'"));
 	return value;
 }
@@ -37,20 +37,35 @@ bool Namespace::Get(const String& field, Value *value) const
 {
 	ObjectLock olock(this);
 
-	auto nsVal = GetAttribute(field);
+	auto nsVal = m_Data.find(field);
 
-	if (!nsVal)
+	if (nsVal == m_Data.end()) {
 		return false;
+	}
 
-	*value =  nsVal->Get(DebugInfo());
+	*value = nsVal->second.Val;
 	return true;
 }
 
-void Namespace::Set(const String& field, const Value& value, bool overrideFrozen)
+void Namespace::Set(const String& field, const Value& value, bool isConst, bool overrideFrozen, const DebugInfo& debugInfo)
 {
 	ObjectLock olock(this);
 
-	return SetFieldByName(field, value, overrideFrozen, DebugInfo());
+	auto nsVal = m_Data.find(field);
+
+	if (nsVal == m_Data.end()) {
+		if (m_Frozen && !overrideFrozen) {
+			BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.", debugInfo));
+		}
+
+		m_Data[field] = NamespaceValue{value, isConst};
+	} else {
+		if (nsVal->second.Const && !overrideFrozen) {
+			BOOST_THROW_EXCEPTION(ScriptError("Constant must not be modified.", debugInfo));
+		}
+
+		nsVal->second.Val = value;
+	}
 }
 
 /**
@@ -69,7 +84,7 @@ bool Namespace::Contains(const String& field) const
 {
 	ObjectLock olock(this);
 
-	return HasOwnField(field);
+	return m_Data.find(field) != m_Data.end();
 }
 
 void Namespace::Remove(const String& field, bool overrideFrozen)
@@ -81,14 +96,19 @@ void Namespace::Remove(const String& field, bool overrideFrozen)
 	}
 
 	if (!overrideFrozen) {
-		auto attr = GetAttribute(field);
+		auto attr = m_Data.find(field);
 
-		if (dynamic_pointer_cast<ConstEmbeddedNamespaceValue>(attr)) {
+		if (attr != m_Data.end() && attr->second.Const) {
 			BOOST_THROW_EXCEPTION(ScriptError("Constants must not be removed."));
 		}
 	}
 
-	RemoveAttribute(field);
+	auto it = m_Data.find(field);
+
+	if (it == m_Data.end())
+		return;
+
+	m_Data.erase(it);
 }
 
 /**
@@ -103,111 +123,31 @@ void Namespace::Freeze() {
 	m_Frozen = true;
 }
 
-void Namespace::RemoveAttribute(const String& field)
-{
-	ObjectLock olock(this);
-
-	Namespace::Iterator it;
-	it = m_Data.find(field);
-
-	if (it == m_Data.end())
-		return;
-
-	m_Data.erase(it);
-}
-
-NamespaceValue::Ptr Namespace::GetAttribute(const String& key) const
-{
-	ObjectLock olock(this);
-
-	auto it = m_Data.find(key);
-
-	if (it == m_Data.end())
-		return nullptr;
-
-	return it->second;
-}
-
-void Namespace::SetAttribute(const String& key, const NamespaceValue::Ptr& nsVal)
-{
-	ObjectLock olock(this);
-
-	m_Data[key] = nsVal;
-}
-
 Value Namespace::GetFieldByName(const String& field, bool, const DebugInfo& debugInfo) const
 {
 	ObjectLock olock(this);
 
-	auto nsVal = GetAttribute(field);
+	auto nsVal = m_Data.find(field);
 
-	if (nsVal)
+	if (nsVal != m_Data.end())
-		return nsVal->Get(debugInfo);
+		return nsVal->second.Val;
 	else
 		return GetPrototypeField(const_cast<Namespace *>(this), field, false, debugInfo); /* Ignore indexer not found errors similar to the Dictionary class. */
 }
 
 void Namespace::SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo)
 {
-	ObjectLock olock(this);
+	Set(field, value, false, overrideFrozen, debugInfo);
-
-	auto nsVal = GetAttribute(field);
-
-	if (!nsVal) {
-		if (m_Frozen && !overrideFrozen) {
-			BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.", debugInfo));
-		}
-
-		if (m_ConstValues) {
-			SetAttribute(field, new ConstEmbeddedNamespaceValue(value));
-		} else {
-			SetAttribute(field, new EmbeddedNamespaceValue(value));
-		}
-	} else {
-		nsVal->Set(value, overrideFrozen, debugInfo);
-	}
 }
 
 bool Namespace::HasOwnField(const String& field) const
 {
-	ObjectLock olock(this);
+	return Contains(field);
-
-	return GetAttribute(field) != nullptr;
 }
 
 bool Namespace::GetOwnField(const String& field, Value *result) const
 {
-	ObjectLock olock(this);
+	return Get(field, result);
-
-	auto nsVal = GetAttribute(field);
-
-	if (!nsVal)
-		return false;
-
-	*result = nsVal->Get(DebugInfo());
-	return true;
-}
-
-EmbeddedNamespaceValue::EmbeddedNamespaceValue(const Value& value)
-	: m_Value(value)
-{ }
-
-Value EmbeddedNamespaceValue::Get(const DebugInfo& debugInfo) const
-{
-	return m_Value;
-}
-
-void EmbeddedNamespaceValue::Set(const Value& value, bool, const DebugInfo&)
-{
-	m_Value = value;
-}
-
-void ConstEmbeddedNamespaceValue::Set(const Value& value, bool overrideFrozen, const DebugInfo& debugInfo)
-{
-	if (!overrideFrozen)
-		BOOST_THROW_EXCEPTION(ScriptError("Constant must not be modified.", debugInfo));
-
-	EmbeddedNamespaceValue::Set(value, overrideFrozen, debugInfo);
 }
 
 Namespace::Iterator Namespace::Begin()

lib/base/namespace.hpp

@@ -15,31 +15,12 @@
 namespace icinga
 {
 
-struct NamespaceValue : public SharedObject
+struct NamespaceValue
 {
-	DECLARE_PTR_TYPEDEFS(NamespaceValue);
+	Value Val;
-
+	bool Const;
-	virtual Value Get(const DebugInfo& debugInfo = DebugInfo()) const = 0;
-	virtual void Set(const Value& value, bool overrideFrozen, const DebugInfo& debugInfo = DebugInfo()) = 0;
 };
 
-struct EmbeddedNamespaceValue : public NamespaceValue
-{
-	EmbeddedNamespaceValue(const Value& value);
-
-	Value Get(const DebugInfo& debugInfo) const override;
-	void Set(const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) override;
-
-private:
-	Value m_Value;
-};
-
-struct ConstEmbeddedNamespaceValue : public EmbeddedNamespaceValue
-{
-	using EmbeddedNamespaceValue::EmbeddedNamespaceValue;
-
-	void Set(const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) override;
-};
 
 /**
  * A namespace.
@@ -51,23 +32,19 @@ class Namespace final : public Object
 public:
 	DECLARE_OBJECT(Namespace);
 
-	typedef std::map<String, NamespaceValue::Ptr>::iterator Iterator;
+	typedef std::map<String, NamespaceValue>::iterator Iterator;
 
-	typedef std::map<String, NamespaceValue::Ptr>::value_type Pair;
+	typedef std::map<String, NamespaceValue>::value_type Pair;
 
 	explicit Namespace(bool constValues = false);
 
 	Value Get(const String& field) const;
 	bool Get(const String& field, Value *value) const;
-	void Set(const String& field, const Value& value, bool overrideFrozen = false);
+	void Set(const String& field, const Value& value, bool isConst = false, bool overrideFrozen = false, const DebugInfo& debugInfo = DebugInfo());
 	bool Contains(const String& field) const;
 	void Remove(const String& field, bool overrideFrozen = false);
 	void Freeze();
 
-	NamespaceValue::Ptr GetAttribute(const String& field) const;
-	void SetAttribute(const String& field, const NamespaceValue::Ptr& nsVal);
-	void RemoveAttribute(const String& field);
-
 	Iterator Begin();
 	Iterator End();
 
@@ -81,7 +58,7 @@ public:
 	static Object::Ptr GetPrototype();
 
 private:
-	std::map<String, NamespaceValue::Ptr> m_Data;
+	std::map<String, NamespaceValue> m_Data;
 	bool m_ConstValues;
 	bool m_Frozen;
 };

lib/base/scriptframe.cpp

@@ -29,18 +29,18 @@ INITIALIZE_ONCE_WITH_PRIORITY([]() {
 	l_SystemNS->Set("BuildHostName", ICINGA_BUILD_HOST_NAME);
 	l_SystemNS->Set("BuildCompilerName", ICINGA_BUILD_COMPILER_NAME);
 	l_SystemNS->Set("BuildCompilerVersion", ICINGA_BUILD_COMPILER_VERSION);
-	globalNS->SetAttribute("System", new ConstEmbeddedNamespaceValue(l_SystemNS));
+	globalNS->Set("System", l_SystemNS, false, true);
 
-	l_SystemNS->SetAttribute("Configuration", new EmbeddedNamespaceValue(new Configuration()));
+	l_SystemNS->Set("Configuration", new Configuration(), false, true);
 
 	l_TypesNS = new Namespace(true);
-	globalNS->SetAttribute("Types", new ConstEmbeddedNamespaceValue(l_TypesNS));
+	globalNS->Set("Types", l_TypesNS, false, true);
 
 	l_StatsNS = new Namespace(true);
-	globalNS->SetAttribute("StatsFunctions", new ConstEmbeddedNamespaceValue(l_StatsNS));
+	globalNS->Set("StatsFunctions", l_StatsNS, false, true);
 
 	l_InternalNS = new Namespace(true);
-	globalNS->SetAttribute("Internal", new ConstEmbeddedNamespaceValue(l_InternalNS));
+	globalNS->Set("Internal", l_InternalNS, false, true);
 }, InitializePriority::CreateNamespaces);
 
 INITIALIZE_ONCE_WITH_PRIORITY([]() {

lib/base/scriptglobal.cpp

@@ -65,7 +65,7 @@ void ScriptGlobal::Set(const String& name, const Value& value, bool overrideFroz
 
 void ScriptGlobal::SetConst(const String& name, const Value& value)
 {
-	GetGlobals()->SetAttribute(name, new ConstEmbeddedNamespaceValue(value));
+	GetGlobals()->Set(name, value, true);
 }
 
 bool ScriptGlobal::Exists(const String& name)
@@ -93,7 +93,7 @@ void ScriptGlobal::WriteToFile(const String& filename)
 
 	ObjectLock olock(m_Globals);
 	for (const Namespace::Pair& kv : m_Globals) {
-		Value value = kv.second->Get();
+		Value value = kv.second.Val;
 
 		if (value.IsObject())
 			value = Convert::ToString(value);

lib/base/serializer.cpp

@@ -131,7 +131,7 @@ static Dictionary::Ptr SerializeNamespace(const Namespace::Ptr& input, int attri
 	ObjectLock olock(input);
 
 	for (const Namespace::Pair& kv : input) {
-		Value val = kv.second->Get();
+		Value val = kv.second.Val;
 		stack.Push(kv.first, val);
 
 		auto serialized (SerializeInternal(val, attributeTypes, stack, dryRun));

lib/base/type.cpp

@@ -54,7 +54,7 @@ std::vector<Type::Ptr> Type::GetAllTypes()
 		ObjectLock olock(typesNS);
 
 		for (const Namespace::Pair& kv : typesNS) {
-			Value value = kv.second->Get();
+			Value value = kv.second.Val;
 
 			if (value.IsObjectType<Type>())
 				types.push_back(value);

lib/config/expression.cpp

@@ -675,20 +675,11 @@ ExpressionResult SetConstExpression::DoEvaluate(ScriptFrame& frame, DebugHint *d
 {
 	auto globals = ScriptGlobal::GetGlobals();
 
-	auto attr = globals->GetAttribute(m_Name);
-
-	if (dynamic_pointer_cast<ConstEmbeddedNamespaceValue>(attr)) {
-		std::ostringstream msgbuf;
-		msgbuf << "Value for constant '" << m_Name << "' was modified. This behaviour is deprecated.\n";
-		ShowCodeLocation(msgbuf, GetDebugInfo(), false);
-		Log(LogWarning, msgbuf.str());
-	}
-
 	ExpressionResult operandres = m_Operand->Evaluate(frame);
 	CHECK_RESULT(operandres);
 	Value operand = operandres.GetValue();
 
-	globals->SetAttribute(m_Name, new ConstEmbeddedNamespaceValue(operand));
+	globals->Set(m_Name, operand, true);
 
 	return Empty;
 }

lib/icinga/cib.cpp

@@ -269,7 +269,7 @@ std::pair<Dictionary::Ptr, Array::Ptr> CIB::GetFeatureStats()
 		ObjectLock olock(statsFunctions);
 
 		for (const Namespace::Pair& kv : statsFunctions)
-			static_cast<Function::Ptr>(kv.second->Get())->Invoke({ status, perfdata });
+			static_cast<Function::Ptr>(kv.second.Val)->Invoke({ status, perfdata });
 	}
 
 	return std::make_pair(status, perfdata);

lib/icinga/icingaapplication.cpp

@@ -54,14 +54,14 @@ void IcingaApplication::StaticInitialize()
 	/* Ensure that the System namespace is already initialized. Otherwise this is a programming error. */
 	VERIFY(systemNS);
 
-	systemNS->Set("ApplicationType", "IcingaApplication");
+	systemNS->Set("ApplicationType", "IcingaApplication", false, true);
-	systemNS->Set("ApplicationVersion", Application::GetAppVersion());
+	systemNS->Set("ApplicationVersion", Application::GetAppVersion(), false, true);
 
 	Namespace::Ptr globalNS = ScriptGlobal::GetGlobals();
 	VERIFY(globalNS);
 
 	l_IcingaNS = new Namespace(true);
-	globalNS->SetAttribute("Icinga", new ConstEmbeddedNamespaceValue(l_IcingaNS));
+	globalNS->Set("Icinga", l_IcingaNS, true);
 }
 
 INITIALIZE_ONCE_WITH_PRIORITY([]() {

lib/icingadb/icingadb-stats.cpp

@@ -24,7 +24,7 @@ Dictionary::Ptr IcingaDB::GetStats()
 
 	for (auto& kv : statsFunctions)
 	{
-		Function::Ptr func = kv.second->Get();
+		Function::Ptr func = kv.second.Val;
 
 		if (!func)
 			BOOST_THROW_EXCEPTION(std::invalid_argument("Invalid status function name."));

lib/remote/variablequeryhandler.cpp

@@ -35,7 +35,7 @@ public:
 			Namespace::Ptr globals = ScriptGlobal::GetGlobals();
 			ObjectLock olock(globals);
 			for (const Namespace::Pair& kv : globals) {
-				addTarget(GetTargetForVar(kv.first, kv.second->Get()));
+				addTarget(GetTargetForVar(kv.first, kv.second.Val));
 			}
 		}
 	}