> On Dec 5, 2025, at 23:33, Peter Eisentraut <[email protected]> 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.)
>
> 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.
>
> - 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.
>
> 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 this patch is not a pure
refactoring but having some runtime impacts. From this perspective, I am
actually keen on #if 0 as Heikki 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/
