Re: commented out code

On 05.12.2025 16:33, Peter Eisentraut wrote:
> There are many PG_GETARG_* calls, mostly around gin, gist, spgist code,
> that are commented out, presumably to indicate that the argument is
> unused and to indicate that it wasn't forgotten or miscounted.  Example:
>
> ...
>     StrategyNumber strategy = (StrategyNumber) PG_GETARG_UINT16(2);
>
>     /* Oid      subtype = PG_GETARG_OID(3); */
>     bool       *recheck = (bool *) PG_GETARG_POINTER(4);
> ...
>
> But keeping commented-out code updated with refactorings and style
> changes is annoying.  (Also note that pgindent forces the blank line.)
>
> One way to address this is to de-comment that code but instead mark the
> variables unused.  That way the compiler can check the code, and the
> purpose is clear to a reader.  Example:
>
>     pg_attribute_unused() Oid subtype = PG_GETARG_OID(3);
>
> (This is the correct placement of the attribute under forward-looking
> C23 alignment.)
>
> I have attached a patch for that.

But that doesn't guarantee that the code is actually optimized away. The
compiler might keep, for example, PG_GETARG_*() code that uses
PG_DETOAST_DATUM_*(), if it cannot to prove that the code is side effect
free.

Did you check if the compiler actually removes all of the code marked
pg_attribute_unused(), especially e.g. the call to PG_GETARG_TEXT_PP()?
How do we avoid regressing when some of the PG_GETARG_*() code is changed?

If we cannot be sure the compiler will actually remove the code, we
could provide PG_GETARG_*_UNUSED() macros that truly won't do anything.

>
> An alternative is to just delete that code.  (No patch attached, but you
> can imagine it.)

> Some particular curious things to check in the patch:
>
> - In contrib/hstore/hstore_gin.c, if I activate the commented out code,
> it causes test failures in the hstore test.  So the commented out code
> is somehow wrong, which seems bad.  Also, maybe there is more wrong code
> like that, but which doesn't trigger test failures right now?
>
> - In src/backend/utils/adt/jsonfuncs.c, those calls, if activated, are
> happening before null checks, so they are not correct.  Also, the "in"
> variable is shadowed later.  So here, deleting the incorrect code is
> probably the best solution in any case.
+1

>
> - In doc/src/sgml/gist.sgml, this is the source of the pattern, it
> actually documents that you should write your functions with the
> commented out code.  We should think about an alternative way to
> document this.  I don't see the "subtype" argument documented anywhere
> in the vicinity of this, so I don't know what the best advice would be.
> Just silently skipping an argument number might be confusing here.

--
David Geier