Reimplement previous NodeRefImpl checks while simplifying code also removing NodeRef.

doc/README.modern_cpp.md

@@ -23,3 +23,5 @@ Only ones mentioned in this document could be used, but as necessities appears,
 - [strongly-typed enum](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2347.pdf)
 - [atomic types and operations](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2427.html)
 - [static_assert](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2004/n1720.html)
+- [decltype](https://en.cppreference.com/w/cpp/language/decltype)
+- [std::is_convertible](https://en.cppreference.com/w/cpp/types/is_convertible)

src/dsql/AggNodes.cpp

@@ -151,8 +151,8 @@ bool AggNode::dsqlAggregateFinder(AggregateFinder& visitor)
 		NodeRefsHolder holder(visitor.getPool());
 		getChildren(holder, true);
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
-			visitor.visit(i->getExpr());
+			visitor.visit(*i);
 
 		localDeepestLevel = visitor.deepestLevel;
 	}
@@ -183,8 +183,8 @@ bool AggNode::dsqlAggregateFinder(AggregateFinder& visitor)
 		NodeRefsHolder holder(visitor.getPool());
 		getChildren(holder, true);
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
-			aggregate |= visitor.visit(i->getExpr());
+			aggregate |= visitor.visit(*i);
 	}
 
 	return aggregate;
@@ -201,8 +201,8 @@ bool AggNode::dsqlAggregate2Finder(Aggregate2Finder& visitor)
 	NodeRefsHolder holder(visitor.getPool());
 	getChildren(holder, true);
 
-	for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+	for (auto i : holder.refs)
-		found |= fieldFinder.visit(i->getExpr());
+		found |= fieldFinder.visit(*i);
 
 	if (!fieldFinder.getField())
 	{
@@ -248,14 +248,14 @@ bool AggNode::dsqlInvalidReferenceFinder(InvalidReferenceFinder& visitor)
 		NodeRefsHolder holder(visitor.dsqlScratch->getPool());
 		getChildren(holder, true);
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
 		{
 			// If there's another aggregate with the same scope_level or
 			// an higher one then it's a invalid aggregate, because
 			// aggregate-functions from the same context can't
 			// be part of each other.
 			if (Aggregate2Finder::find(visitor.dsqlScratch->getPool(), visitor.context->ctx_scope_level,
-					FIELD_MATCH_TYPE_EQUAL, false, i->getExpr()))
+					FIELD_MATCH_TYPE_EQUAL, false, *i))
 			{
 				// Nested aggregate functions are not allowed
 				ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-104) <<
@@ -287,8 +287,11 @@ ValueExprNode* AggNode::dsqlFieldRemapper(FieldRemapper& visitor)
 	NodeRefsHolder holder(visitor.getPool());
 	getChildren(holder, true);
 
-	for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+	for (auto i : holder.refs)
-		i->remap(visitor);
+	{
+		if (*i)
+			*i = (*i)->dsqlFieldRemapper(visitor);
+	}
 
 	return this;
 }
@@ -326,7 +329,7 @@ void AggNode::genBlr(DsqlCompilerScratch* dsqlScratch)
 
 		unsigned count = 0;
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
 		{
 			if (*i)
 				++count;
@@ -335,10 +338,10 @@ void AggNode::genBlr(DsqlCompilerScratch* dsqlScratch)
 		dsqlScratch->appendUChar(UCHAR(count));
 	}
 
-	for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+	for (auto i : holder.refs)
 	{
 		if (*i)
-			GEN_expr(dsqlScratch, i->getExpr());
+			GEN_expr(dsqlScratch, *i);
 	}
 }
 

src/dsql/ExprNodes.cpp

@@ -185,35 +185,6 @@ static void setParameterInfo(dsql_par* parameter, const dsql_ctx* context);
 //--------------------
 
 
-void NodeRef::pass2(thread_db* tdbb, CompilerScratch* csb)
-{
-	internalPass2(tdbb, csb);
-
-	ExprNode* node = getExpr();
-
-	// Bind values of invariant nodes to top-level RSE (if present)
-	if (node && (node->nodFlags & ExprNode::FLAG_INVARIANT))
-	{
-		if (csb->csb_current_nodes.hasData())
-		{
-			RseNode* topRseNode = nodeAs<RseNode>(csb->csb_current_nodes[0]);
-			fb_assert(topRseNode);
-
-			if (!topRseNode->rse_invariants)
-			{
-				topRseNode->rse_invariants =
-					FB_NEW_POOL(*tdbb->getDefaultPool()) VarInvariantArray(*tdbb->getDefaultPool());
-			}
-
-			topRseNode->rse_invariants->add(node->impureOffset);
-		}
-	}
-}
-
-
-//--------------------
-
-
 void Printable::print(NodePrinter& printer) const
 {
 	NodePrinter subPrinter(printer.getIndent() + 1);
@@ -267,9 +238,9 @@ bool ExprNode::dsqlMatch(DsqlCompilerScratch* dsqlScratch, const ExprNode* other
 
 	const auto* j = otherHolder.refs.begin();
 
-	for (const auto& i : thisHolder.refs)
+	for (const auto i : thisHolder.refs)
 	{
-		if (!i != !*j || !PASS1_node_match(dsqlScratch, i.getExpr(), j->getExpr(), ignoreMapCast))
+		if (!*i != !**j || !PASS1_node_match(dsqlScratch, *i, **j, ignoreMapCast))
 			return false;
 
 		++j;
@@ -295,12 +266,12 @@ bool ExprNode::sameAs(CompilerScratch* csb, const ExprNode* other, bool ignoreSt
 
 	const auto* j = otherHolder.refs.begin();
 
-	for (const auto& i : thisHolder.refs)
+	for (const auto i : thisHolder.refs)
 	{
-		if (!i && !*j)
+		if (!*i && !**j)
 			continue;
 
-		if (!i || !*j || !i.getExpr()->sameAs(csb, j->getExpr(), ignoreStreams))
+		if (!*i || !**j || !(*i)->sameAs(csb, **j, ignoreStreams))
 			return false;
 
 		++j;
@@ -314,9 +285,9 @@ bool ExprNode::possiblyUnknown(OptimizerBlk* opt)
 	NodeRefsHolder holder(opt->getPool());
 	getChildren(holder, false);
 
-	for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+	for (auto i : holder.refs)
 	{
-		if (*i && i->getExpr()->possiblyUnknown(opt))
+		if (*i && (*i)->possiblyUnknown(opt))
 			return true;
 	}
 
@@ -328,9 +299,9 @@ bool ExprNode::unmappable(CompilerScratch* csb, const MapNode* mapNode, StreamTy
 	NodeRefsHolder holder(csb->csb_pool);
 	getChildren(holder, false);
 
-	for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+	for (auto i : holder.refs)
 	{
-		if (*i && !i->getExpr()->unmappable(csb, mapNode, shellStream))
+		if (*i && !(*i)->unmappable(csb, mapNode, shellStream))
 			return false;
 	}
 
@@ -342,10 +313,10 @@ void ExprNode::collectStreams(CompilerScratch* csb, SortedStreamList& streamList
 	NodeRefsHolder holder(csb->csb_pool);
 	getChildren(holder, false);
 
-	for (const NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+	for (auto i : holder.refs)
 	{
 		if (*i)
-			i->getExpr()->collectStreams(csb, streamList);
+			(*i)->collectStreams(csb, streamList);
 	}
 }
 
@@ -355,9 +326,9 @@ bool ExprNode::computable(CompilerScratch* csb, StreamType stream,
 	NodeRefsHolder holder(csb->csb_pool);
 	getChildren(holder, false);
 
-	for (auto& i : holder.refs)
+	for (auto i : holder.refs)
 	{
-		if (i && !i.getExpr()->computable(csb, stream, allowOnlyCurrentStream))
+		if (*i && !(*i)->computable(csb, stream, allowOnlyCurrentStream))
 			return false;
 	}
 
@@ -369,10 +340,10 @@ void ExprNode::findDependentFromStreams(const OptimizerRetrieval* optRet, Sorted
 	NodeRefsHolder holder(optRet->getPool());
 	getChildren(holder, false);
 
-	for (auto& i : holder.refs)
+	for (auto i : holder.refs)
 	{
-		if (i)
+		if (*i)
-			i.getExpr()->findDependentFromStreams(optRet, streamList);
+			(*i)->findDependentFromStreams(optRet, streamList);
 	}
 }
 
@@ -381,10 +352,10 @@ ExprNode* ExprNode::pass1(thread_db* tdbb, CompilerScratch* csb)
 	NodeRefsHolder holder(csb->csb_pool);
 	getChildren(holder, false);
 
-	for (auto& i : holder.refs)
+	for (auto i : holder.refs)
 	{
-		if (i)
+		if (*i)
-			i.pass1(tdbb, csb);
+			doPass1(tdbb, csb, i);
 	}
 
 	return this;
@@ -395,10 +366,32 @@ ExprNode* ExprNode::pass2(thread_db* tdbb, CompilerScratch* csb)
 	NodeRefsHolder holder(csb->csb_pool);
 	getChildren(holder, false);
 
-	for (auto& i : holder.refs)
+	for (auto i : holder.refs)
 	{
-		if (i)
+		if (!*i)
-			i.pass2(tdbb, csb);
+			continue;
+
+		doPass2(tdbb, csb, i);
+
+		ExprNode* node = *i;
+
+		// Bind values of invariant nodes to top-level RSE (if present)
+		if (node && (node->nodFlags & ExprNode::FLAG_INVARIANT))
+		{
+			if (csb->csb_current_nodes.hasData())
+			{
+				RseNode* topRseNode = nodeAs<RseNode>(csb->csb_current_nodes[0]);
+				fb_assert(topRseNode);
+
+				if (!topRseNode->rse_invariants)
+				{
+					topRseNode->rse_invariants =
+						FB_NEW_POOL(*tdbb->getDefaultPool()) VarInvariantArray(*tdbb->getDefaultPool());
+				}
+
+				topRseNode->rse_invariants->add(node->impureOffset);
+			}
+		}
 	}
 
 	return this;
@@ -9051,8 +9044,8 @@ bool OverNode::dsqlAggregateFinder(AggregateFinder& visitor)
 		NodeRefsHolder holder(visitor.getPool());
 		aggExpr->getChildren(holder, true);
 
-		for (auto& child : holder.refs)
+		for (auto child : holder.refs)
-			aggregate |= visitor.visit(child.getExpr());
+			aggregate |= visitor.visit(*child);
 	}
 	else
 		aggregate |= visitor.visit(aggExpr);
@@ -9114,10 +9107,10 @@ ValueExprNode* OverNode::dsqlFieldRemapper(FieldRemapper& visitor)
 	NodeRefsHolder holder(visitor.getPool());
 	aggNode->getChildren(holder, true);
 
-	for (auto& child : holder.refs)
+	for (auto child : holder.refs)
 	{
 		if (Aggregate2Finder::find(visitor.getPool(), visitor.context->ctx_scope_level, FIELD_MATCH_TYPE_EQUAL,
-				true, child.getExpr()))
+				true, *child))
 		{
 			ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-104) <<
 					  Arg::Gds(isc_dsql_agg_nested_err));
@@ -9137,8 +9130,11 @@ ValueExprNode* OverNode::dsqlFieldRemapper(FieldRemapper& visitor)
 			NodeRefsHolder holder(visitor.getPool());
 			aggNode->getChildren(holder, true);
 
-			for (auto& child : holder.refs)
+			for (auto child : holder.refs)
-				child.remap(visitor);
+			{
+				if (*child)
+					*child = (*child)->dsqlFieldRemapper(visitor);
+			}
 
 			doDsqlFieldRemapper(visitor, window);
 		}

src/dsql/Nodes.h

@@ -395,64 +395,6 @@ template <typename To, typename From> static bool nodeIs(const NestConst<From>&
 }
 
 
-// Stores a reference to a specialized ExprNode.
-// This class and NodeRefImpl exists for nodes to replace themselves (eg. pass1) in a type-safe way.
-class NodeRef
-{
-public:
-	NodeRef()
-		: ptr(NULL)
-	{
-	}
-
-	template <typename T>
-	explicit NodeRef(const NestConst<T>& node)
-		: ptr(NULL)
-	{
-		static_assert(std::is_base_of<ExprNode, T>::value, "T must be derived from ExprNode");
-
-		T** aPtr = const_cast<T**> (node.getAddress());
-		ptr = reinterpret_cast<ExprNode**> (aPtr);
-
-		fb_assert(ptr);
-	}
-
-	bool operator !() const
-	{
-		return !getExpr();
-	}
-
-	operator bool() const
-	{
-		return getExpr() != NULL;
-	}
-
-	ExprNode* getExpr()
-	{
-		return *ptr;
-	}
-
-	const ExprNode* getExpr() const
-	{
-		return *ptr;
-	}
-
-	void remap(FieldRemapper& visitor);
-
-	void pass1(thread_db* tdbb, CompilerScratch* csb)
-	{
-		DmlNode::doPass1(tdbb, csb, ptr);
-	}
-
-	void pass2(thread_db* tdbb, CompilerScratch* csb);
-
-protected:
-	void internalPass2(thread_db* tdbb, CompilerScratch* csb);
-
-private:
-	ExprNode** ptr;
-};
-
 class NodeRefsHolder : public Firebird::PermanentStorage
 {
 public:
@@ -462,18 +404,41 @@ public:
 	{
 	}
 
-	~NodeRefsHolder()
-	{
-	}
-
-
 	template <typename T> void add(const NestConst<T>& node)
 	{
-		refs.add(NodeRef(node));
+		static_assert(std::is_base_of<ExprNode, T>::value, "T must be derived from ExprNode");
+
+		static_assert(
+			std::is_convertible<
+				decltype(const_cast<T*>(node.getObject())->pass1(
+					(thread_db*) nullptr, (CompilerScratch*) nullptr)),
+				decltype(const_cast<T*>(node.getObject()))
+			>::value,
+			"pass1 problem");
+
+		static_assert(
+			std::is_convertible<
+				decltype(const_cast<T*>(node.getObject())->pass2(
+					(thread_db*) nullptr, (CompilerScratch*) nullptr)),
+				decltype(const_cast<T*>(node.getObject()))
+			>::value,
+			"pass2 problem");
+
+		static_assert(
+			std::is_convertible<
+				decltype(const_cast<T*>(node.getObject())->dsqlFieldRemapper(*(FieldRemapper*) nullptr)),
+				decltype(const_cast<T*>(node.getObject()))
+			>::value,
+			"dsqlFieldRemapper problem");
+
+		T** ptr = const_cast<T**> (node.getAddress());
+		fb_assert(ptr);
+
+		refs.add(reinterpret_cast<ExprNode**>(ptr));
 	}
 
 public:
-	Firebird::HalfStaticArray<NodeRef, 8> refs;
+	Firebird::HalfStaticArray<ExprNode**, 8> refs;
 };
 
 
@@ -600,8 +565,8 @@ public:
 		NodeRefsHolder holder(visitor.getPool());
 		getChildren(holder, true);
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
-			ret |= visitor.visit(i->getExpr());
+			ret |= visitor.visit(*i);
 
 		return ret;
 	}
@@ -613,8 +578,8 @@ public:
 		NodeRefsHolder holder(visitor.getPool());
 		getChildren(holder, true);
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
-			ret |= visitor.visit(i->getExpr());
+			ret |= visitor.visit(*i);
 
 		return ret;
 	}
@@ -626,8 +591,8 @@ public:
 		NodeRefsHolder holder(visitor.getPool());
 		getChildren(holder, true);
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
-			ret |= visitor.visit(i->getExpr());
+			ret |= visitor.visit(*i);
 
 		return ret;
 	}
@@ -639,8 +604,8 @@ public:
 		NodeRefsHolder holder(visitor.dsqlScratch->getPool());
 		getChildren(holder, true);
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
-			ret |= visitor.visit(i->getExpr());
+			ret |= visitor.visit(*i);
 
 		return ret;
 	}
@@ -652,8 +617,8 @@ public:
 		NodeRefsHolder holder(visitor.getPool());
 		getChildren(holder, true);
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
-			ret |= visitor.visit(i->getExpr());
+			ret |= visitor.visit(*i);
 
 		return ret;
 	}
@@ -663,8 +628,11 @@ public:
 		NodeRefsHolder holder(visitor.getPool());
 		getChildren(holder, true);
 
-		for (NodeRef* i = holder.refs.begin(); i != holder.refs.end(); ++i)
+		for (auto i : holder.refs)
-			i->remap(visitor);
+		{
+			if (*i)
+				*i = (*i)->dsqlFieldRemapper(visitor);
+		}
 
 		return this;
 	}
@@ -730,18 +698,6 @@ public:
 };
 
 
-inline void NodeRef::remap(FieldRemapper& visitor)
-{
-	if (*ptr)
-		*ptr = (*ptr)->dsqlFieldRemapper(visitor);
-}
-
-inline void NodeRef::internalPass2(thread_db* tdbb, CompilerScratch* csb)
-{
-	ExprNode::doPass2(tdbb, csb, ptr);
-}
-
-
 class BoolExprNode : public ExprNode
 {
 public:

src/dsql/StmtNodes.cpp

@@ -9435,7 +9435,7 @@ static void dsqlSetParameterName(DsqlCompilerScratch* dsqlScratch, ExprNode* exp
 			exprNode->getChildren(holder, true);
 
 			for (auto ref : holder.refs)
-				dsqlSetParameterName(dsqlScratch, ref.getExpr(), fld_node, relation);
+				dsqlSetParameterName(dsqlScratch, *ref, fld_node, relation);
 
 			break;
 		}