13.03.2019 2:37, Adriano dos Santos Fernandes wrote:
On 12/03/2019 17:22, Vlad Khorsun wrote:
Let me demonstrate it. With this (wrong) patch now the code compiles:
--------------
diff --git a/src/dsql/ExprNodes.cpp b/src/dsql/ExprNodes.cpp
index 4ff5253a2b..26f826ae09 100644
--- a/src/dsql/ExprNodes.cpp
+++ b/src/dsql/ExprNodes.cpp
@@ -10836,6 +10836,7 @@ void SubQueryNode::getChildren(NodeRefsHolder&
holder, bool dsql) const
holder.add(value1);
holder.add(value2);
+ holder.add(test);
}
string SubQueryNode::internalPrint(NodePrinter& printer) const
diff --git a/src/dsql/ExprNodes.h b/src/dsql/ExprNodes.h
index 3478e4056b..38469e826e 100644
--- a/src/dsql/ExprNodes.h
+++ b/src/dsql/ExprNodes.h
@@ -1860,6 +1860,7 @@ public:
NestConst<ValueExprNode> value1;
NestConst<ValueExprNode> value2;
NestConst<SubQuery> subQuery;
+ NestConst<CastNode> test;
};
--------------
While previously it correctly detected the problem, as a specialized
node cannot become some other node not inherited from it.
Please, define "specialized node". I assumed it is descendants of the
ExprNode, but it seems it is not enough. What makes node "specialized" ?
...
As the code demonstrate, "NestConst<CastNode> test" should always point
to a CastNode (or a possible descendant). CastNode::pass1 currently
returns this (a CastNode, but doesn't matter), but it declares return
type as ValueExprNode so it can return other things that is not a
CastNode descendant. Some other nodes does that.
And if does that, "NestConst<CastNode> test" will point to a wrong thing
without the compiler detect when doPass1 (or pass2, remap, etc) is
called. NodeRefImpl was there to detect that wrong cases.
Sorry, it is still not clear - how one could know if node requires
"specialization" ?
Imagine a new developer who want to create new kind of Nodes - what is formal
criteria of "specialized" node ?
Currently it could be detected by return type of dsqlFieldRemapper (and
probably dsqlPass, copy, pass1, pass2) virtual function(s) - if at least one
of them returns type of current class (i.e. overrides return type of base
class).
Is it correct ?
Anyway, it will be good to have documented formal criteria of "specialized"
node.
If the change really increases performance, then I suppose it should be
conditionally defined based on DEBUG build, so debug could use NodeRef
with virtual method and NodeRefImpl, and release something like
currently (non-virtual method and no NodeRefImpl usage).
According to profiler, new\delete of NodeRefImpl takes near 28% of
request compilation time.
It's should be possible to rewrite the code to have the checks and avoid
the performance problem.
So far i see two possible solutions:
1. what you offer (DEBUG build with old code and RELEASE build with new one)
I can live with it, but want to try something that will not depend on build
mode, thus:
2. mark specialized classes, for example in this way:
class BoolExprNode : public ExprNode
{
public:
typedef BoolExprNode mark_t;
...
}
class ValueExprNode : public ExprNode
{
public:
typedef ValueExprNode mark_t;
...
}
// ... and few more classes, full list:
// WindowClause, Frame, FrameExtent, BoolExprNode, ValueExprNode,
RecordSourceNode, ValueListNode, RecSourceListNode, RseNode
and check this mark presence at compile time:
explicit NodeRef(const NestConst<T>& node)
: ptr(NULL)
{
static_assert(std::is_base_of_v<ExprNode, T>, "T must be derived from
ExprNode");
static_assert(std::is_same_v<T, T::mark_t>, "T must be specialized ExprNode
class");
...
}
It also allows to remove not needed overridden functions, such as:
virtual ValueExprNode* dsqlFieldRemapper(FieldRemapper& visitor)
{
ExprNode::dsqlFieldRemapper(visitor);
return this;
}
What do you think ?
Regards,
Vlad
Firebird-Devel mailing list, web interface at
https://lists.sourceforge.net/lists/listinfo/firebird-devel
