Обсуждение: [MASSMAIL]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.camel@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
Вложения
On Tue, Apr 9, 2024 at 2:10 PM Jeff Davis <pgsql@j-davis.com> 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
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
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
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
On Tue, Apr 9, 2024 at 3:30 PM Jeff Davis <pgsql@j-davis.com> 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
On Tue, Apr 9, 2024 at 3:33 PM Jeff Davis <pgsql@j-davis.com> 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