Thread: lwlocknames.h beautification attempt
Currently the contents of lwlocknames.h look like this:
#define ShmemIndexLock (&MainLWLockArray[1].lock)
#define OidGenLock (&MainLWLockArray[2].lock)
#define XidGenLock (&MainLWLockArray[3].lock)
#define ProcArrayLock (&MainLWLockArray[4].lock)
#define SInvalReadLock (&MainLWLockArray[5].lock)
...
This makes it a bit hard to read, since there is no attempt at vertical alignment along the opening parentheses. It gets worse if the editor performs syntax highlighting, and the various colored characters in the definitions appear haphazardly arranged.
I propose the following change to the generation script, generate-lwlocknames.pl
- print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n";
+ printf $h "#define %-30s %s\n", "${lockname}Lock", "(&MainLWLockArray[$lockidx].lock)";
which produces the lock names in this format
#define ShmemIndexLock (&MainLWLockArray[1].lock)
#define OidGenLock (&MainLWLockArray[2].lock)
#define XidGenLock (&MainLWLockArray[3].lock)
#define ProcArrayLock (&MainLWLockArray[4].lock)
#define SInvalReadLock (&MainLWLockArray[5].lock)
...
Best regards,
Gurjeet
http://Gurje.et
#define ShmemIndexLock (&MainLWLockArray[1].lock)
#define OidGenLock (&MainLWLockArray[2].lock)
#define XidGenLock (&MainLWLockArray[3].lock)
#define ProcArrayLock (&MainLWLockArray[4].lock)
#define SInvalReadLock (&MainLWLockArray[5].lock)
...
This makes it a bit hard to read, since there is no attempt at vertical alignment along the opening parentheses. It gets worse if the editor performs syntax highlighting, and the various colored characters in the definitions appear haphazardly arranged.
Had this been hand-written, I can imagine the author making an attempt at aligning the definitions, but since this is a generated file, the lack of that attempt is understandable.
I propose the following change to the generation script, generate-lwlocknames.pl
- print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n";
+ printf $h "#define %-30s %s\n", "${lockname}Lock", "(&MainLWLockArray[$lockidx].lock)";
which produces the lock names in this format
#define ShmemIndexLock (&MainLWLockArray[1].lock)
#define OidGenLock (&MainLWLockArray[2].lock)
#define XidGenLock (&MainLWLockArray[3].lock)
#define ProcArrayLock (&MainLWLockArray[4].lock)
#define SInvalReadLock (&MainLWLockArray[5].lock)
...
Yet another format, which I prefer, can be achieved by right-aligning the lock names. In addition to aligning the definitions, it also aligns the 'Lock' suffix in all the names. But as they say beauty is in the eye of the beholder, so this one may be actively disliked by others.
- print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n";
+ printf $h "#define %30s %s\n", "${lockname}Lock", "(&MainLWLockArray[$lockidx].lock)";
+ printf $h "#define %30s %s\n", "${lockname}Lock", "(&MainLWLockArray[$lockidx].lock)";
#define ShmemIndexLock (&MainLWLockArray[1].lock)
#define OidGenLock (&MainLWLockArray[2].lock)
#define XidGenLock (&MainLWLockArray[3].lock)
#define ProcArrayLock (&MainLWLockArray[4].lock)
#define SInvalReadLock (&MainLWLockArray[5].lock)
#define OidGenLock (&MainLWLockArray[2].lock)
#define XidGenLock (&MainLWLockArray[3].lock)
#define ProcArrayLock (&MainLWLockArray[4].lock)
#define SInvalReadLock (&MainLWLockArray[5].lock)
...
The lockname column needs to be at least 30 chars wide to accommodate the longest of the lock names 'DynamicSharedMemoryControlLock'.
Best regards,
Gurjeet
http://Gurje.et
Gurjeet Singh <gurjeet@singh.im> writes:
> I propose the following change to the generation script,
> generate-lwlocknames.pl
> ...
> which produces the lock names in this format
> #define ShmemIndexLock (&MainLWLockArray[1].lock)
> #define OidGenLock (&MainLWLockArray[2].lock)
> #define XidGenLock (&MainLWLockArray[3].lock)
> #define ProcArrayLock (&MainLWLockArray[4].lock)
> #define SInvalReadLock (&MainLWLockArray[5].lock)
This looks reasonably in line with project style ...
> Yet another format, which I prefer, can be achieved by right-aligning the
> lock names.
> #define ShmemIndexLock (&MainLWLockArray[1].lock)
> #define OidGenLock (&MainLWLockArray[2].lock)
> #define XidGenLock (&MainLWLockArray[3].lock)
> #define ProcArrayLock (&MainLWLockArray[4].lock)
> #define SInvalReadLock (&MainLWLockArray[5].lock)
... but that doesn't. I challenge you to provide even one example
of that layout in our source tree, or to explain why it's better.
regards, tom lane
On Sat, Mar 1, 2025 at 10:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Gurjeet Singh <gurjeet@singh.im> writes: > > I propose the following change to the generation script, > > generate-lwlocknames.pl > > ... > > which produces the lock names in this format > > > #define ShmemIndexLock (&MainLWLockArray[1].lock) > > #define OidGenLock (&MainLWLockArray[2].lock) > > #define XidGenLock (&MainLWLockArray[3].lock) > > #define ProcArrayLock (&MainLWLockArray[4].lock) > > #define SInvalReadLock (&MainLWLockArray[5].lock) > > This looks reasonably in line with project style ... Should I create a commitfest entry for this patch, or is it uncontroversial enough and small enough to not warrant that? > > Yet another format, which I prefer, can be achieved by right-aligning the > > lock names. > > > #define ShmemIndexLock (&MainLWLockArray[1].lock) > > #define OidGenLock (&MainLWLockArray[2].lock) > > #define XidGenLock (&MainLWLockArray[3].lock) > > #define ProcArrayLock (&MainLWLockArray[4].lock) > > #define SInvalReadLock (&MainLWLockArray[5].lock) > > ... but that doesn't. I challenge you to provide even one example > of that layout in our source tree, or to explain why it's better. I haven't seen this style in any other project, let alone in Postgres. I just mentioned it since I personally liked it slightly better after trying a couple of different styles, because of the aligned suffix in the lock names. Best regards, Gurjeet http://Gurje.et
Gurjeet Singh <gurjeet@singh.im> writes:
> On Sat, Mar 1, 2025 at 10:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This looks reasonably in line with project style ...
> Should I create a commitfest entry for this patch, or is it
> uncontroversial enough and small enough to not warrant that?
The controversy would be more about whether it's worth bothering
with. In the past we've taken some care to ensure that generated
files would pass pgindent's ideas of correct formatting, but
not more than that. AFAICT from some quick tests, pgindent has
no opinions about layout of #define lines.
regards, tom lane
On Sun, Mar 2, 2025 at 1:10 AM Gurjeet Singh <gurjeet@singh.im> wrote:
> I propose the following change to the generation script, generate-lwlocknames.pl
>
> - print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n";
> + printf $h "#define %-30s %s\n", "${lockname}Lock", "(&MainLWLockArray[$lockidx].lock)";
-1. That seems worse to me.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 2025-Mar-01, Gurjeet Singh wrote:
> I propose the following change to the generation script,
> generate-lwlocknames.pl
>
> - print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n";
> + printf $h "#define %-30s %s\n", "${lockname}Lock", "(&MainLWLockArray[$lockidx].lock)";
>
> which produces the lock names in this format
>
> #define ShmemIndexLock (&MainLWLockArray[1].lock)
> #define OidGenLock (&MainLWLockArray[2].lock)
This format seems more than acceptable. It'd be nice to avoid printf
for such a small thing; but at least we can use the %-specifiers only
where necessary, so I'd do this instead:
diff --git a/src/backend/storage/lmgr/generate-lwlocknames.pl b/src/backend/storage/lmgr/generate-lwlocknames.pl
index 084da2ea6e0..2927f144905 100644
--- a/src/backend/storage/lmgr/generate-lwlocknames.pl
+++ b/src/backend/storage/lmgr/generate-lwlocknames.pl
@@ -108,7 +108,8 @@ while (<$lwlocklist>)
$continue = ",\n";
# Add a "Lock" suffix to each lock name, as the C code depends on that
- print $h "#define ${lockname}Lock (&MainLWLockArray[$lockidx].lock)\n";
+ printf $h "#define %-32s (&MainLWLockArray[$lockidx].lock)\n",
+ "${lockname}Lock";
}
die
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/