Thread: Harmonizing pg_bsd_indent parameter names
Attached patch harmonizes pg_bsd_indent's function parameter names, so that they match the names used in corresponding function definitions. I have been putting this off because I wasn't sure that the policy should be the same for pg_bsd_indent. Is there any reason to think that this will create more work down the line? It seems like it might, due to some kind of need to keep pg_bsd_indent's consistent with upstream BSD indent. But probably not. The patch is pretty small, in any case. I'd like to push this patch now. It's generally easier to be strict about these inconsistencies. My clang-tidy workflow doesn't automatically filter out various special cases requiring subjective judgement, so leaving stuff like this unfixed creates more work down the road. -- Peter Geoghegan
Attachment
On Wed, Jun 12, 2024 at 05:14:44PM -0400, Peter Geoghegan wrote: > I have been putting this off because I wasn't sure that the policy > should be the same for pg_bsd_indent. Is there any reason to think > that this will create more work down the line? It seems like it might, > due to some kind of need to keep pg_bsd_indent's consistent with > upstream BSD indent. But probably not. The patch is pretty small, in > any case. I would be surprised if this 2-line patch created anything approaching a significant amount of work down the road. FWIW commit 10d34fe already changed one line in indent.c. > I'd like to push this patch now. It's generally easier to be strict > about these inconsistencies. My clang-tidy workflow doesn't > automatically filter out various special cases requiring subjective > judgement, so leaving stuff like this unfixed creates more work down > the road. Are these the only remaining inconsistencies? -- nathan
Peter Geoghegan <pg@bowt.ie> writes:
> Attached patch harmonizes pg_bsd_indent's function parameter names, so
> that they match the names used in corresponding function definitions.
Hmm, these aren't really harmonizing inconsistencies, but overruling
somebody's style decision to leave parameter names out of the extern
declarations. That's a style I don't like personally, but some do.
> I have been putting this off because I wasn't sure that the policy
> should be the same for pg_bsd_indent. Is there any reason to think
> that this will create more work down the line? It seems like it might,
> due to some kind of need to keep pg_bsd_indent's consistent with
> upstream BSD indent.
We are, at least in theory, trying to stay within hailing distance
of the upstream; that's the primary reason why we've not touched
the indentation style of pg_bsd_indent itself. Still, two lines
is not going to make much of a difference in whether patches can
be passed back and forth (whereas reindentation would kill that
somewhat thoroughly).
Anyway, after chewing on it for a few minutes, no objection here.
regards, tom lane
On Wed, Jun 12, 2024 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Peter Geoghegan <pg@bowt.ie> writes: > > Attached patch harmonizes pg_bsd_indent's function parameter names, so > > that they match the names used in corresponding function definitions. > > Hmm, these aren't really harmonizing inconsistencies, but overruling > somebody's style decision to leave parameter names out of the extern > declarations. That's a style I don't like personally, but some do. It was my understanding that that was considered just as bad as (or equivalent to) a mechanical inconsistency. At least for code that was written from scratch for Postgres (as opposed to vendored in Postgres) was concerned. We dealt with this during the initial work on bulk harmonizing code. For example, we made Henry Spencer's regex code follow Postgres coding standards (in commit bc2187ed). The regex code was a little different to pg_bsd_indent. There wasn't the same need to keep up with an upstream. That's why I thought I'd ask about it before acting. > We are, at least in theory, trying to stay within hailing distance > of the upstream; that's the primary reason why we've not touched > the indentation style of pg_bsd_indent itself. Still, two lines > is not going to make much of a difference in whether patches can > be passed back and forth (whereas reindentation would kill that > somewhat thoroughly). Got it. > Anyway, after chewing on it for a few minutes, no objection here. Cool. Will push this shortly, then. -- Peter Geoghegan
On Wed, Jun 12, 2024 at 5:32 PM Nathan Bossart <nathandbossart@gmail.com> wrote: > I would be surprised if this 2-line patch created anything approaching a > significant amount of work down the road. FWIW commit 10d34fe already > changed one line in indent.c. I missed that. > > I'd like to push this patch now. It's generally easier to be strict > > about these inconsistencies. My clang-tidy workflow doesn't > > automatically filter out various special cases requiring subjective > > judgement, so leaving stuff like this unfixed creates more work down > > the road. > > Are these the only remaining inconsistencies? No, but they're just about the only remaining inconsistencies that seem fixable. The vast majority of the remaining inconsistencies (reported by run-clang-tidy.py, using only its readability-inconsistent-declaration-parameter-name and readability-named-parameter checks) fit into one of two categories (similar categories): 1. Functions such as GUC_yy_scan_string. These are functions where the only way to fix the complained-about inconsistency is to change the way that upstream Gnu flex generates its scanner C code. There doesn't seem to be a built-in option that influences this behavior. 2. Postgres C code that uses the C preprocessor in the style of C++ templates. In practice category 2 just means users of simplehash.h. There are a couple of those, and I get at least one warning for each of them. There is also one oddball case, not quite in either category. This involves zic.c's declaration of link(), when it should actually just be using the #include <unistd.h> declaration. That's another weird upstream code thing -- this isn't exactly fully under our control. I've avoided doing anything about that, but perhaps I should have proposed a patch for that, too (it's fairly similar to pg_bsd_indent). What do you think of that idea? Personally I don't care all that much about the machine-generated code (I'd fix it if there was a straightforward way to do so, but there doesn't seem to be, so meh). I use clangd's clang-tidy integration. It won't complain about these cases anyway (it doesn't recognize that .l files and .y files contain some C code). -- Peter Geoghegan
On Wed, Jun 12, 2024 at 05:59:14PM -0400, Peter Geoghegan wrote: > There is also one oddball case, not quite in either category. This > involves zic.c's declaration of > link(), when it should actually just be using the #include <unistd.h> > declaration. That's another weird upstream code thing -- this isn't > exactly fully under our control. I've avoided doing anything about > that, but perhaps I should have proposed a patch for that, too (it's > fairly similar to pg_bsd_indent). What do you think of that idea? That one seems to be synchronized somewhat regularly, and I haven't been the one doing the synchronizing, so we might want to be a little more cautious there. But I do see a couple of commits that have touched it (e.g., 235c0f6, c4f8e89, 0245f8d). At a glance, it looks like the link() stuff might be intended for Windows. I see we have our own version in win32link.c, so your idea to remove it in favor of the unistd.h declaration seems like it ought to work. -- nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Wed, Jun 12, 2024 at 05:59:14PM -0400, Peter Geoghegan wrote:
>> There is also one oddball case, not quite in either category. This
>> involves zic.c's declaration of
>> link(), when it should actually just be using the #include <unistd.h>
>> declaration.
> That one seems to be synchronized somewhat regularly, and I haven't been
> the one doing the synchronizing, so we might want to be a little more
> cautious there.
Yeah. I'm overdue for another sync with upstream --- I'm dreading
that a little bit because they've been aggressively "modernizing"
their code and I fear it will be painful.
[ ... click click ... git pull ... ] It looks like the way that
reads now in upstream is
#if !HAVE_POSIX_DECLS
extern int getopt(int argc, char * const argv[],
const char * options);
extern int link(const char * target, const char * linkname);
extern char * optarg;
extern int optind;
#endif
We could probably assume that we'll treat their code as though
HAVE_POSIX_DECLS is true and so this whole stanza goes away.
But I'd just as soon not think about it until I have the energy
to do that sync. Unless somebody else is hot to do it (if so,
see the notes at src/timezone/README), let's leave this be
for now.
regards, tom lane
On Wed, Jun 12, 2024 at 9:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > We could probably assume that we'll treat their code as though > HAVE_POSIX_DECLS is true and so this whole stanza goes away. > But I'd just as soon not think about it until I have the energy > to do that sync. Unless somebody else is hot to do it (if so, > see the notes at src/timezone/README), let's leave this be > for now. Understood. Thanks. -- Peter Geoghegan