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