Thread: Remove unnecessary static specifier
Hi,
When reviewing patch [1], I find that the static class specifier is unnecessary
for the variables sp and ep in the function px_crypt_md5().
diff --git a/contrib/pgcrypto/crypt-md5.c b/contrib/pgcrypto/crypt-md5.c
index d38721a1010..3d17b2340fe 100644
--- a/contrib/pgcrypto/crypt-md5.c
+++ b/contrib/pgcrypto/crypt-md5.c
@@ -36,8 +36,8 @@ px_crypt_md5(const char *pw, const char *salt, char *passwd, unsigned dstlen)
static char *magic = "$1$"; /* This string is magic for this algorithm.
* Having it this way, we can get better later
* on */
- static char *p;
- static const char *sp,
+ char *p;
+ const char *sp,
*ep;
unsigned char final[MD5_SIZE];
int sl,
I also find that FreeBSD removed this specifier in [2].
Should we remove this?
[1] https://www.postgresql.org/message-id/c763235a2757e2f5f9e3e27268b9028349cef659.camel%40oopsware.de
[2] https://reviews.freebsd.org/D7306
--
Regrads,
Japin Li
> On 5 Feb 2025, at 13:30, Japin Li <japinli@hotmail.com> wrote: > When reviewing patch [1], I find that the static class specifier is unnecessary > for the variables sp and ep in the function px_crypt_md5(). From a quick first inspection (and running the tests with the patch applied) I agree with this, these variables do not need to be static. I'll stare a bit more at this to make sure but seems like the right patch. -- Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
> From a quick first inspection (and running the tests with the patch applied) I
> agree with this, these variables do not need to be static. I'll stare a bit
> more at this to make sure but seems like the right patch.
+1. All three of those variables are visibly assigned to before any
other reference, so they cannot carry data across calls of the
function.
While we're at it, could we make the adjacent "magic" string be
"static const char *magic"? (Probably needs a couple more
"const" modifiers at use sites, too.)
regards, tom lane
> On 5 Feb 2025, at 17:05, Tom Lane <tgl@sss.pgh.pa.us> wrote: > While we're at it, could we make the adjacent "magic" string be > "static const char *magic"? (Probably needs a couple more > "const" modifiers at use sites, too.) Good point, from the link referenced it's clear that FreeBSD has made that change as well. I'll fix that at the same time. -- Daniel Gustafsson
On Wed, 05 Feb 2025 at 18:17, Daniel Gustafsson <daniel@yesql.se> wrote: >> On 5 Feb 2025, at 17:05, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> While we're at it, could we make the adjacent "magic" string be >> "static const char *magic"? (Probably needs a couple more >> "const" modifiers at use sites, too.) > > Good point, from the link referenced it's clear that FreeBSD has made that > change as well. I'll fix that at the same time. > Oh, I didn't notice this. +1. -- Regrads, Japin Li
> On 6 Feb 2025, at 02:51, Japin Li <japinli@hotmail.com> wrote: > > On Wed, 05 Feb 2025 at 18:17, Daniel Gustafsson <daniel@yesql.se> wrote: >>> On 5 Feb 2025, at 17:05, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >>> While we're at it, could we make the adjacent "magic" string be >>> "static const char *magic"? (Probably needs a couple more >>> "const" modifiers at use sites, too.) >> >> Good point, from the link referenced it's clear that FreeBSD has made that >> change as well. I'll fix that at the same time. > > Oh, I didn't notice this. +1. Committed, thanks! -- Daniel Gustafsson