John Naylor <[email protected]> writes:
> On 3/21/18, Tom Lane <[email protected]> wrote:
>> regproc aggmtransfn BKI_DEFAULT(-) BKI_LOOKUP(pg_proc);
>> regproc aggminvtransfn BKI_DEFAULT(-) BKI_LOOKUP(pg_proc);
>> regproc aggmfinalfn BKI_DEFAULT(-) BKI_LOOKUP(pg_proc);
>> I'm not sure if there's anything much to be done about this. BKI_DEFAULT
>> isn't so bad, but additional annotations get unreadable fast. Maybe
>> BKI_LOOKUP was a bad idea after all, and we should just invent more
>> Oid-equivalent typedef names.
> Well, until version 7, I used "fake" type aliases that only genbki.pl
> could see. The C compiler and postgres.bki could only see the actual
> Oid/oid type. Perhaps it was a mistake to model their appearance after
> regproc (regtype, etc), because that was misleading. Maybe something
> more obviously transient like 'lookup_typeoid? I'm leaning towards
> this idea.
Looking at this again, I think a big chunk of the readability problem here
is just from the fact that we have long, similar-looking lines tightly
packed. If it were reformatted to have comment lines and whitespace
between, it might not look nearly as bad.
> Another possibility is to teach the pgindent pre_/post_indent
> functions to preserve annotation formatting, but I'd rather not add
> yet another regex to that script. Plus, over the next 10+ years, I
> could see people adding several more BKI_* macros, leading to
> readability issues regardless of formatting, so maybe we should nip
> this one in the bud.
Well, whether or not we invent BKI_LOOKUP, the need for other kinds
of annotations isn't likely to be lessened.
I wondered whether we could somehow convert the format into multiple
lines, say
regproc aggmfinalfn
BKI_DEFAULT(-)
BKI_LOOKUP(pg_proc);
but some quick experimentation was discouraging: either Emacs' C
syntax mode, or pgindent, or both would make a hash of it. It
wasn't great from the vertical-space-consumption standpoint either.
Anyway, for the moment I'd stick with BKI_LOOKUP rather than undoing
that work. I think it's a more transparent way of saying what we
want than the magic-OID-typedefs approach. The formatting issue is
just a mild annoyance, and it's not really BKI_LOOKUP's fault anyway.
While I'm thinking of it --- I noticed one or two places where you
had "BKI_DEFAULT(\0)". That coding scares me a bit --- gcc seems to
tolerate it, but other C compilers might feel that \0 is not a valid
preprocessing token, or it might confuse some editors' syntax highlight
rules. I'd rather write cases like this as "BKI_DEFAULT('\0')". IOW,
the argument should be a valid C identifier, number, or string literal.
regards, tom lane
