Re: ancient sequence point bug

-----Original Message-----
From: pgsql-hackers-owner(at)postgresql(dot)org [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Tom Lane
Sent: Tuesday, April 16, 2013 7:52 PM
To: Peter Eisentraut
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [HACKERS] ancient sequence point bug

>Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> This code in bootstrap.c contains a sequence point violation (or
>> whatever that is really called):
>
>> while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)
>> {
>> (*app)->am_oid = HeapTupleGetOid(tup);
>> memmove((char *) &(*app++)->am_typ,
>> (char *) GETSTRUCT(tup),
>> sizeof((*app)->am_typ));
>> }
>
>What exactly is the violation? sizeof() is a side-effect-free compile time constant, and the first value to be passed to memmove seems perfectly well defined.

Unless it is C99 and the object is a VLA, which must be evaluated at runtime. I guess that (*app)->am_typ is not a VLA, especially since PostgreSQL does not require C99 to compile.

>I grant that this is not terribly good coding style, but I don't believe there's an actual risk here.

I agree that there is no risk in the sizeof operator.
According to my understanding, even this is legal:

unsigned long long *p = 0;
size_t stupid = sizeof (*p);
printf("size of a long long is %u\n", (unsigned) stupid);

But I also guess that most compilers will have a cow when scanning it.

>> In commit 1aebc361, another place in the same file was fixed like this:
>
>Well, I don't really remember why I did that twelve years ago, but seeing that there are a number of purely-cosmetic changes in that commit, I'm thinking it was only meant to be cosmetic.
>
>I certainly have no objection to making this code look more like that code, I'm just not seeing that it's a bug.

You are right. It's not a bug. However, I would not be surprised if GCC or CLANG would shriek at the higher warning levels, usually not being able to apply artificial intelligence to solve problems that seem simple to humans.
In the interest of "quiet compiles" equivalent code that does not produce a warning seems like a good idea. IMO-YMMV.

> regards, tom lane
>
>
>