Re: commented out code

On 05/12/2025 17: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.
> 
> An alternative is to just delete that code.  (No patch attached, but you 
> can imagine it.)

#if 0
    Oid      subtype = PG_GETARG_OID(3);
#endif

is yet another option. It keeps the indentation, although you won't get 
the compiler checking.

> 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?

I'm guessing that the commented out code detoasts the arguments.

> - 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.

Wow, that jsonb_set_lax() function is difficult to follow. Especially 
the "jsonb_delete_path(fcinfo)" call seems pretty accidental to work, 
because jsonb_delete_path() just happens to have the same two arguments.

> - 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.

Hmm, yeah, the right thing to do would be to actually document the 
'subtype'. I don't remember what it is off the top of my head.

- Heikki