Re: %TYPE and array declaration patch review

Hello

2011/11/28 Greg Smith <greg(at)2ndquadrant(dot)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

>