On Thu, Mar 5, 2009 at 1:12 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre@commandprompt.com> writes:
>> >> Seems doable.
>>
>> > Attached.
>>
>> The TWO_MEMBER_SCANKEYWORD business seems a bit dangerous --- if the
>> header file is read without having #defined that correctly, bad things
>> will happen. It might be better to leave that out, always define the
>> struct the same, and just have pg_dump define PG_KEYWORD to fill the
>> value field with zero. Given alignment considerations, you're not
>> saving any space by omitting the field anyhow.
>
> Fixed.
>
> I also added #include type.h to the ecpg keywords.c file, which means we
> don't need to redefine YYSTYPE at all on any of the three keywords.c
> file. Looks cleaner overall.
>
> Hopefully this is the last version of this patch.
FWIW gcc does this kind of trick all over the place. They have lists
of various types of objects, not unlike our nodes and define them in
.h files which contain _just_ the equivalent of PG_KEYWORD. Then they
include those files in various places with the macro defined to do
different things. So for example they define an enum, an array for
external consumption, an array for internal consumption, and in many
places even switch statements with trivial bits of code from the macro
too.
If we're going to go this route I think it does make sense to move the
"const ScanKeyword ScanKeywords[] = {" preamble and to live with the
PG_KEYWORD definition. Even if we're not planning to have any other
kinds of macro definitions aside from ScanKeywords arrays today it
would be nice to have the flexibility and in any case I don't really
like the action-at-a-distance of having a macro definition in one
place which depends on the definition in another place.
--
greg