Re: commented out code - Mailing list pgsql-hackers

> On Dec 5, 2025, at 23:33, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> There are many PG_GETARG_* calls, mostly around gin, gist, spgist code, that are commented out, presumably to
indicatethat 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
forcesthe blank line.) 
>
> One way to address this is to de-comment that code but instead mark the variables unused.  That way the compiler can
checkthe 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.)
>
> 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.
Sothe commented out code is somehow wrong, which seems bad.  Also, maybe there is more wrong code like that, but which
doesn'ttrigger 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
inany case. 
>
> - In doc/src/sgml/gist.sgml, this is the source of the pattern, it actually documents that you should write your
functionswith 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
silentlyskipping an argument number might be confusing here. 
>
> Thoughts?
> <0001-Mark-commented-out-code-as-unused.patch>


Looking at the definition of pg_attribute_unused:
```
/* only GCC supports the unused attribute */
#ifdef __GNUC__
#define pg_attribute_unused() __attribute__((unused))
#else
#define pg_attribute_unused()
#endif
```

Only GCC really supports it. Even with gcc, based on my understanding, for example:

+    pg_attribute_unused() text *query = PG_GETARG_TEXT_PP(2);

The assignment to “query” will still happen, “__attribute__((unused))" only hides “unused variable” compile warning. So
thispatch is not a pure refactoring but having some runtime impacts. From this perspective, I am actually keen on #if 0
asHeikki suggested. If we go along with #if 0, then the 3 curious issues would not happen. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/