[prev in list] [next in list] [prev in thread] [next in thread] 

List:       pgsql-hackers
Subject:    Re: [HACKERS] %TYPE and array declaration patch review
From:       Pavel Stehule <pavel.stehule () gmail ! com>
Date:       2011-11-30 15:42:32
Message-ID: CAFj8pRCU+tF5zhx=B1FEU4OLSTL6nbwo1PX9mmW2Oru8ujbu2A () mail ! gmail ! com
[Download RAW message or body]

Hello

2011/11/28 Greg Smith <greg@2ndquadrant.com>:
> I'm trying to find someone for the "[PL/pgSQL] %TYPE and array declaration -
> second patch" patch submitted recently:
>   https://commitfest.postgresql.org/action/patch_view?id=666
>
> Not too many people work on the PL/pgSQL code, and I see you reviewed an
> earlier version of this patch.   Do you think you could find time to review
> the update to it as well?
>

This patch is not applyed cleanly now

bash-4.2$ patch -p1 < type_array.patch
patching file doc/src/sgml/plpgsql.sgml
patching file src/pl/plpgsql/src/gram.y
Hunk #5 succeeded at 2540 (offset -12 lines).
Hunk #6 succeeded at 2554 (offset -12 lines).
Hunk #7 succeeded at 2595 (offset -12 lines).
patching file src/pl/plpgsql/src/pl_comp.c
Hunk #1 succeeded at 1586 (offset 2 lines).
Hunk #2 succeeded at 1883 (offset 2 lines).
Hunk #3 FAILED at 1901.
Hunk #4 succeeded at 2034 (offset 3 lines).
1 out of 4 hunks FAILED -- saving rejects to file
src/pl/plpgsql/src/pl_comp.c.rej
patching file src/pl/plpgsql/src/plpgsql.h
Hunk #2 succeeded at 895 (offset 8 lines).
patching file src/test/regress/expected/plpgsql.out
patching file src/test/regress/sql/plpgsql.sql


I dislike using macros without parameters

+#define word1 strVal(linitial(idents))
+#define word2 strVal(lsecond(idents))
+#define word3 strVal(lthird(idents))

and
-               nse = plpgsql_ns_lookup(plpgsql_ns_top(), false,
-
strVal(linitial(idents)),
-
strVal(lsecond(idents)),
-                                                               NULL,
+               var = (PLpgSQL_var *) plpgsql_get_variable2(
+                                                                       word1,
+                                                                       word2,
+
 PLPGSQL_NSTYPE_VAR,
+                                                                       NULL);

This change is useless, and smudges a code - a list operations are
well known and is not neccessary hide it.

macros

#define linitial_str(lc) strVal(linitial(lc))
#define lsecond_str(lc) strVal(lsecond(lc))
#define lthird_str(lc) strVal(lthird(lc))

these macros should be defined only once per module - #undef is not
used usually in pg source code, don't use it in this case

Regress tests are really large - it is question if about 900 lines is
necessary - should be more compact

Regards

Pavel



>

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[prev in list] [next in list] [prev in thread] [next in thread]