Обсуждение: Remove Value node struct
While trying to refactor the node support in various ways, the Value node is always annoying. The Value node struct is a weird construct. It is its own node type, but most of the time, it actually has a node type of Integer, Float, String, or BitString. As a consequence, the struct name and the node type don't match most of the time, and so it has to be treated specially a lot. There doesn't seem to be any value in the special construct. There is very little code that wants to accept all Value variants but nothing else (and even if it did, this doesn't provide any convenient way to check it), and most code wants either just one particular node type (usually String), or it accepts a broader set of node types besides just Value. This change removes the Value struct and node type and replaces them by separate Integer, Float, String, and BitString node types that are proper node types and structs of their own and behave mostly like normal node types. Also, this removes the T_Null node tag, which was previously also a possible variant of Value but wasn't actually used outside of the Value contained in A_Const. Replace that by an isnull field in A_Const.
Вложения
On Wed, Aug 25, 2021 at 9:20 AM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > This change removes the Value struct and node type and replaces them > by separate Integer, Float, String, and BitString node types that are > proper node types and structs of their own and behave mostly like > normal node types. +1. I noticed this years ago and never thought of doing anything about it. I'm glad you did think of it... -- Robert Haas EDB: http://www.enterprisedb.com
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes: > While trying to refactor the node support in various ways, the Value > node is always annoying. […] > This change removes the Value struct and node type and replaces them > by separate Integer, Float, String, and BitString node types that are > proper node types and structs of their own and behave mostly like > normal node types. This looks like a nice cleanup overall, independent of any future refactoring. > Also, this removes the T_Null node tag, which was previously also a > possible variant of Value but wasn't actually used outside of the > Value contained in A_Const. Replace that by an isnull field in > A_Const. However, the patch adds: > +typedef struct Null > +{ > + NodeTag type; > + char *val; > +} Null; which doesn't seem to be used anywhere. Is that a leftoverf from an intermediate development stage? - ilmari
On Wed, Aug 25, 2021 at 9:49 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Aug 25, 2021 at 9:20 AM Peter Eisentraut > <peter.eisentraut@enterprisedb.com> wrote: > > This change removes the Value struct and node type and replaces them > > by separate Integer, Float, String, and BitString node types that are > > proper node types and structs of their own and behave mostly like > > normal node types. > > +1. I noticed this years ago and never thought of doing anything about > it. I'm glad you did think of it... +1, it also bothered me in the past.
Agree to the motive and +1 for the concept. At Wed, 25 Aug 2021 15:00:13 +0100, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote in > However, the patch adds: > > > +typedef struct Null > > +{ > > + NodeTag type; > > + char *val; > > +} Null; > > which doesn't seem to be used anywhere. Is that a leftoverf from an > intermediate development stage? +1 Looks like so, it can be simply removed. 0001 looks fine to me. 0002: there's an "integer Value node" in gram.y: 7776. - n = makeFloatConst(v->val.str, location); + n = (Node *) makeFloatConst(castNode(Float, v)->val, location); makeFloatConst is Node* so the cast doesn't seem needed. The same can be said for Int and String Consts. This looks like a confustion with makeInteger and friends. + else if (IsA(obj, Integer)) + _outInteger(str, (Integer *) obj); + else if (IsA(obj, Float)) + _outFloat(str, (Float *) obj); I felt that the type enames are a bit confusing as they might be too generic, or too close with the corresponding binary types. - Node *arg; /* a (Value *) or a (TypeName *) */ + Node *arg; Mmm. It's a bit pity that we lose the generic name for the value nodes. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 30.08.21 04:13, Kyotaro Horiguchi wrote: >> However, the patch adds: >> >>> +typedef struct Null >>> +{ >>> + NodeTag type; >>> + char *val; >>> +} Null; >> >> which doesn't seem to be used anywhere. Is that a leftoverf from an >> intermediate development stage? > > +1 Looks like so, it can be simply removed. fixed > 0002: > there's an "integer Value node" in gram.y: 7776. fixed > - n = makeFloatConst(v->val.str, location); > + n = (Node *) makeFloatConst(castNode(Float, v)->val, location); > > makeFloatConst is Node* so the cast doesn't seem needed. The same can > be said for Int and String Consts. This looks like a confustion with > makeInteger and friends. fixed > + else if (IsA(obj, Integer)) > + _outInteger(str, (Integer *) obj); > + else if (IsA(obj, Float)) > + _outFloat(str, (Float *) obj); > > I felt that the type enames are a bit confusing as they might be too > generic, or too close with the corresponding binary types. > > > - Node *arg; /* a (Value *) or a (TypeName *) */ > + Node *arg; > > Mmm. It's a bit pity that we lose the generic name for the value nodes. Not sure what you mean here.
Вложения
At Tue, 7 Sep 2021 11:22:24 +0200, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in > On 30.08.21 04:13, Kyotaro Horiguchi wrote: > > + else if (IsA(obj, Integer)) > > + _outInteger(str, (Integer *) obj); > > + else if (IsA(obj, Float)) > > + _outFloat(str, (Float *) obj); > > I felt that the type enames are a bit confusing as they might be too > > generic, or too close with the corresponding binary types. > > - Node *arg; /* a (Value *) or a (TypeName *) */ > > + Node *arg; > > Mmm. It's a bit pity that we lose the generic name for the value > > nodes. > > Not sure what you mean here. The member arg loses the information on what kind of nodes are to be stored there. Concretely it just removes the comment "a (Value *) or a (TypeName *)". If the (Value *) were expanded in a straight way, the comment would be "a (Integer *), (Float *), (String *), (BitString *), or (TypeName *)". I supposed that the member loses the comment because it become too long. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 08.09.21 04:04, Kyotaro Horiguchi wrote: > At Tue, 7 Sep 2021 11:22:24 +0200, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in >> On 30.08.21 04:13, Kyotaro Horiguchi wrote: >>> + else if (IsA(obj, Integer)) >>> + _outInteger(str, (Integer *) obj); >>> + else if (IsA(obj, Float)) >>> + _outFloat(str, (Float *) obj); >>> I felt that the type enames are a bit confusing as they might be too >>> generic, or too close with the corresponding binary types. >>> - Node *arg; /* a (Value *) or a (TypeName *) */ >>> + Node *arg; >>> Mmm. It's a bit pity that we lose the generic name for the value >>> nodes. >> >> Not sure what you mean here. > > The member arg loses the information on what kind of nodes are to be > stored there. Concretely it just removes the comment "a (Value *) or a > (TypeName *)". If the (Value *) were expanded in a straight way, the > comment would be "a (Integer *), (Float *), (String *), (BitString *), > or (TypeName *)". I supposed that the member loses the comment because > it become too long. Ok, I added the comment back in in a modified form. The patches have been committed now. Thanks.