Обсуждение: 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.