Thread
Commits
-
Remove Value node struct
- 639a86e36aae 15.0 landed
-
Remove useless casts
- cbdf75bf8053 15.0 landed
-
Remove Value node struct
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-25T13:19:49Z
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.
-
Re: Remove Value node struct
Robert Haas <robertmhaas@gmail.com> — 2021-08-25T13:49:18Z
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
-
Re: Remove Value node struct
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2021-08-25T14:00:13Z
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 -
Re: Remove Value node struct
Julien Rouhaud <rjuju123@gmail.com> — 2021-08-28T03:41:48Z
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.
-
Re: Remove Value node struct
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-08-30T02:13:58Z
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 -
Re: Remove Value node struct
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-09-07T09:22:24Z
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. -
Re: Remove Value node struct
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-08T02:04:55Z
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
-
Re: Remove Value node struct
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-09-09T07:23:54Z
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.