Re: Unportable coding in reorderbuffer.h

On 2014-03-05 17:40:56 -0500, Tom Lane wrote:
> I don't believe that this is legal per C90:
>
> typedef struct ReorderBufferChange
> {
> XLogRecPtr lsn;
>
> /* type of change */
> union
> {
> enum ReorderBufferChangeType action;
> /* do not leak internal enum values to the outside */
> int action_internal;
> };
>
> ...
>
> That union field needs a name.

You're absolutely right.

> Our project standard is we compile on C90 compilers, and we're not
> moving that goalpost just to save you a couple of keystrokes.

While it's a good idea to try to save keystrokes the actual reason is
that I just had somehow forgotten that it hasn't always been
supported. I think it's not even C99, but C11...

> By the time you get done fixing the portability issue, I suspect you
> won't have a union at all for the first case.

You might be right. I'd rather not leak the internal enum values to the
public though, so there are two choices:
* define as enum, but store values in the that aren't actually valid
members. IIRC that's legal, even if not pretty. Anything outside
reorderbuffer.c doesn't ever see the values that aren't enum values.
* define it as an int, but suggest casting to the enum inside switch()
in examples/docs.

> I'm not real sure that
> you need a union for the second case either --- is it really important
> to shave a few bytes from the size of this struct? So you don't
> necessarily need to do a notation change for the second union.

I think it's allocated frequently enough to warrant that
unfortunately. Changing that will be fun.

> What drew my attention to it was an older gcc version complaining
> thusly: [errors]

And there I was, suprised it hadn't turned the buildfarm red.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services