Re: simplehash.h: "SH_SCOPE static" causes warnings
On Tue, Apr 9, 2024 at 3:33 PM Jeff Davis wrote: > Should I go ahead and commit something like that now, or hold it until > the other thread concludes, or hold it until the July CF? I think it's fine to commit it now if it makes it usefully easier to fix an open item, and otherwise it should wait until next cycle. But I also wonder if we shouldn't just keep using static inline everywhere. I'm guessing if we do this people are just going to make random decisions about which one to use every time this comes up. -- Robert Haas EDB: http://www.enterprisedb.com
Re: simplehash.h: "SH_SCOPE static" causes warnings
On Tue, Apr 9, 2024 at 3:30 PM Jeff Davis wrote: > Pages of warnings is not ideal, though. We should either support > "SH_SCOPE static", or have some kind of useful #error that makes it > clear that we don't support it (and/or don't think it's a good idea). Fair. > > I'm not sure that I like the idea of just ignoring the > > warnings, for fear that the compiler might not actually remove the > > code for the unused functions from the resulting binary. But I'm not > > an expert in this area either, so maybe I'm wrong. > > In a simple "hello world" test with an unreferenced static function, it > doesn't seem to be a problem at -O2. I suppose it could be with some > compiler somewhere, or perhaps in a more complex scenario, but it would > seem strange to me. OK. -- Robert Haas EDB: http://www.enterprisedb.com
Re: simplehash.h: "SH_SCOPE static" causes warnings
Hi, On Tue, 2024-04-09 at 11:56 -0700, Andres Freund wrote: > FWIW, with just about any modern-ish compiler just using "inline" > doesn't > actually force inlining, it just changes the cost model to make it > more > likely. OK. In the linked thread, I didn't see a good reason to encourage the compiler to inline the code. Only one caller uses the hash table, so my instinct would be that the code for maniuplating it should not be inlined. But "extern" (which is the scope now) is certainly not right, so "static" made the most sense to me. > > I'm not opposed. I'd however at least add a comment explaining why > this is > being used. Arguably it doesn't make sense to add it to *_create(), > as without > that there really isn't a point in having a simplehash instantiation. > Might > make it slightly easier to notice obsoleted uses of simplehash. That's a good idea that preserves some utility for the warnings. Should I go ahead and commit something like that now, or hold it until the other thread concludes, or hold it until the July CF? Regards, Jeff Davis
Re: simplehash.h: "SH_SCOPE static" causes warnings
On Tue, 2024-04-09 at 14:49 -0400, Robert Haas wrote: > Hmm. I'm pretty sure that I've run into this problem, but I concluded > that I should use either "static inline" or "extern" and didn't think > any more of it. Pages of warnings is not ideal, though. We should either support "SH_SCOPE static", or have some kind of useful #error that makes it clear that we don't support it (and/or don't think it's a good idea). > I'm not sure that I like the idea of just ignoring the > warnings, for fear that the compiler might not actually remove the > code for the unused functions from the resulting binary. But I'm not > an expert in this area either, so maybe I'm wrong. In a simple "hello world" test with an unreferenced static function, it doesn't seem to be a problem at -O2. I suppose it could be with some compiler somewhere, or perhaps in a more complex scenario, but it would seem strange to me. Regards, Jeff Davis
Re: simplehash.h: "SH_SCOPE static" causes warnings
Hi, On 2024-04-09 11:10:15 -0700, Jeff Davis wrote: > The reason I'm suggesting it there is because the hash table is used > only for the indexed binary heap, not an ordinary binary heap, so I'd > like to leave it up to the compiler whether to do any inlining or not. FWIW, with just about any modern-ish compiler just using "inline" doesn't actually force inlining, it just changes the cost model to make it more likely. > If someone thinks the attached patch is a good change to commit now, > please let me know. Otherwise, I'll recommend "static inline" in the > above thread and leave the attached patch to be considered for v18. I'm not opposed. I'd however at least add a comment explaining why this is being used. Arguably it doesn't make sense to add it to *_create(), as without that there really isn't a point in having a simplehash instantiation. Might make it slightly easier to notice obsoleted uses of simplehash. Greetings, Andres Freund
Re: simplehash.h: "SH_SCOPE static" causes warnings
On Tue, Apr 9, 2024 at 2:10 PM Jeff Davis wrote: > If using "SH_SCOPE static" with simplehash.h, it causes a bunch of > warnings about functions that are defined but not used. It's simple > enough to fix by appending pg_attribute_unused() to the declarations > (attached). Hmm. I'm pretty sure that I've run into this problem, but I concluded that I should use either "static inline" or "extern" and didn't think any more of it. I'm not sure that I like the idea of just ignoring the warnings, for fear that the compiler might not actually remove the code for the unused functions from the resulting binary. But I'm not an expert in this area either, so maybe I'm wrong. -- Robert Haas EDB: http://www.enterprisedb.com
simplehash.h: "SH_SCOPE static" causes warnings
If using "SH_SCOPE static" with simplehash.h, it causes a bunch of warnings about functions that are defined but not used. It's simple enough to fix by appending pg_attribute_unused() to the declarations (attached). There are currently no callers that use "SH_SCOPE static", but I'm suggesting its use in the thread below as a cleanup to a recently- committed feature: https://www.postgresql.org/message-id/791d98f474e518387d09eb390b8a12f265d130cc.ca...@j-davis.com The reason I'm suggesting it there is because the hash table is used only for the indexed binary heap, not an ordinary binary heap, so I'd like to leave it up to the compiler whether to do any inlining or not. If someone thinks the attached patch is a good change to commit now, please let me know. Otherwise, I'll recommend "static inline" in the above thread and leave the attached patch to be considered for v18. Regards, Jeff Davis From 198235448a009a46812eac4854d760af76c85139 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Tue, 9 Apr 2024 10:42:05 -0700 Subject: [PATCH v1 1/2] simplehash.h: declare with pg_attribute_unused(). If SH_SCOPE is static, avoid warnings for unused functions. --- src/include/lib/simplehash.h | 30 +++--- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h index 3e1b1f9461..7d93d68e93 100644 --- a/src/include/lib/simplehash.h +++ b/src/include/lib/simplehash.h @@ -188,62 +188,62 @@ typedef struct SH_ITERATOR /* externally visible function prototypes */ #ifdef SH_RAW_ALLOCATOR /* _hash _create(uint32 nelements, void *private_data) */ -SH_SCOPE SH_TYPE *SH_CREATE(uint32 nelements, void *private_data); +SH_SCOPE SH_TYPE *SH_CREATE(uint32 nelements, void *private_data) pg_attribute_unused(); #else /* * _hash _create(MemoryContext ctx, uint32 nelements, * void *private_data) */ SH_SCOPE SH_TYPE *SH_CREATE(MemoryContext ctx, uint32 nelements, - void *private_data); + void *private_data) pg_attribute_unused(); #endif /* void _destroy(_hash *tb) */ -SH_SCOPE void SH_DESTROY(SH_TYPE * tb); +SH_SCOPE void SH_DESTROY(SH_TYPE * tb) pg_attribute_unused(); /* void _reset(_hash *tb) */ -SH_SCOPE void SH_RESET(SH_TYPE * tb); +SH_SCOPE void SH_RESET(SH_TYPE * tb) pg_attribute_unused(); /* void _grow(_hash *tb, uint64 newsize) */ -SH_SCOPE void SH_GROW(SH_TYPE * tb, uint64 newsize); +SH_SCOPE void SH_GROW(SH_TYPE * tb, uint64 newsize) pg_attribute_unused(); /* *_insert(_hash *tb, key, bool *found) */ -SH_SCOPE SH_ELEMENT_TYPE *SH_INSERT(SH_TYPE * tb, SH_KEY_TYPE key, bool *found); +SH_SCOPE SH_ELEMENT_TYPE *SH_INSERT(SH_TYPE * tb, SH_KEY_TYPE key, bool *found) pg_attribute_unused(); /* * *_insert_hash(_hash *tb, key, uint32 hash, * bool *found) */ SH_SCOPE SH_ELEMENT_TYPE *SH_INSERT_HASH(SH_TYPE * tb, SH_KEY_TYPE key, - uint32 hash, bool *found); + uint32 hash, bool *found) pg_attribute_unused(); /* *_lookup(_hash *tb, key) */ -SH_SCOPE SH_ELEMENT_TYPE *SH_LOOKUP(SH_TYPE * tb, SH_KEY_TYPE key); +SH_SCOPE SH_ELEMENT_TYPE *SH_LOOKUP(SH_TYPE * tb, SH_KEY_TYPE key) pg_attribute_unused(); /* *_lookup_hash(_hash *tb, key, uint32 hash) */ SH_SCOPE SH_ELEMENT_TYPE *SH_LOOKUP_HASH(SH_TYPE * tb, SH_KEY_TYPE key, - uint32 hash); + uint32 hash) pg_attribute_unused(); /* void _delete_item(_hash *tb, *entry) */ -SH_SCOPE void SH_DELETE_ITEM(SH_TYPE * tb, SH_ELEMENT_TYPE * entry); +SH_SCOPE void SH_DELETE_ITEM(SH_TYPE * tb, SH_ELEMENT_TYPE * entry) pg_attribute_unused(); /* bool _delete(_hash *tb, key) */ -SH_SCOPE bool SH_DELETE(SH_TYPE * tb, SH_KEY_TYPE key); +SH_SCOPE bool SH_DELETE(SH_TYPE * tb, SH_KEY_TYPE key) pg_attribute_unused(); /* void _start_iterate(_hash *tb, _iterator *iter) */ -SH_SCOPE void SH_START_ITERATE(SH_TYPE * tb, SH_ITERATOR * iter); +SH_SCOPE void SH_START_ITERATE(SH_TYPE * tb, SH_ITERATOR * iter) pg_attribute_unused(); /* * void _start_iterate_at(_hash *tb, _iterator *iter, * uint32 at) */ -SH_SCOPE void SH_START_ITERATE_AT(SH_TYPE * tb, SH_ITERATOR * iter, uint32 at); +SH_SCOPE void SH_START_ITERATE_AT(SH_TYPE * tb, SH_ITERATOR * iter, uint32 at) pg_attribute_unused(); /* *_iterate(_hash *tb, _iterator *iter) */ -SH_SCOPE SH_ELEMENT_TYPE *SH_ITERATE(SH_TYPE * tb, SH_ITERATOR * iter); +SH_SCOPE SH_ELEMENT_TYPE *SH_ITERATE(SH_TYPE * tb, SH_ITERATOR * iter) pg_attribute_unused(); /* void _stat(_hash *tb */ -SH_SCOPE void SH_STAT(SH_TYPE * tb); +SH_SCOPE void SH_STAT(SH_TYPE * tb) pg_attribute_unused(); #endif /* SH_DECLARE */ -- 2.34.1
