Horton

Thread

Commits

  1. Phase 2 pgindent run for v12.

  2. ANSI-ify a few straggler K&R-style function definitions.

  1. Emacs vs pg_indent's weird indentation for function declarations

    Thomas Munro <thomas.munro@enterprisedb.com> — 2019-01-27T22:10:33Z

    Hello,

Using either the .dir-locals.el settings or the "more complete"
src/tools/editors/emacs.samples, I have never convinced Emacs to
produce multi-line function declarations in .h files that satisfy
pg_indent.

For example, latch.c has the following definition:

int
WaitEventSetWait(WaitEventSet *set, long timeout,
                 WaitEvent *occurred_events, int nevents,
                 uint32 wait_event_info)

And now let's see the declaration in latch.h:

extern int WaitEventSetWait(WaitEventSet *set, long timeout,
                 WaitEvent *occurred_events, int nevents,
                 uint32 wait_event_info);

(I replaced all tabs with four space here; if you're reading in a
monospace font, you'll see that the first arguments off all lines
begin in the same column in the definition by not in the declaration.)

For a while I've been baffled by that: the first arguments of later
lines don't line up with that of the first line, but they're also not
in a constant column (it varies from function to function), and it's
also not caused by 8-space vs 4-space confusion.  It was only when I
put those two things next to each other just now in this email that I
finally spotted the logic it's using: if you remove "extern int " then
the later lines line up with the first argument of the top line.  This
works for other examples I looked at too.  Huh.

That's ... annoying.  I wish indent wouldn't do that, because it means
that my declarations get moved around every time I write code.  But if
it's not possible to change that for whatever technical or political
reason, I wonder if it's possible to teach Emacs to understand that
weird rule...

-- 
Thomas Munro
http://www.enterprisedb.com

  2. Re: Emacs vs pg_indent's weird indentation for function declarations

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-28T05:28:30Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> For a while I've been baffled by that: the first arguments of later
> lines don't line up with that of the first line, but they're also not
> in a constant column (it varies from function to function), and it's
> also not caused by 8-space vs 4-space confusion.  It was only when I
> put those two things next to each other just now in this email that I
> finally spotted the logic it's using: if you remove "extern int " then
> the later lines line up with the first argument of the top line.  This
> works for other examples I looked at too.  Huh.

Yeah.  I suspect that the underlying cause is that pgindent doesn't
really distinguish function declarations from definitions, at least
not when it comes time to indent the lines after the first one.

> That's ... annoying.  I wish indent wouldn't do that, because it means
> that my declarations get moved around every time I write code.

If you can fix it, I'd vote for accepting the patch.  I don't personally
have the desire to dig into the indent code that much ...

			regards, tom lane

  3. Re: Emacs vs pg_indent's weird indentation for function declarations

    Michael Paquier <michael@paquier.xyz> — 2019-01-28T07:08:42Z

    On Mon, Jan 28, 2019 at 12:28:30AM -0500, Tom Lane wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> That's ... annoying.  I wish indent wouldn't do that, because it means
>> that my declarations get moved around every time I write code.
> 
> If you can fix it, I'd vote for accepting the patch.  I don't personally
> have the desire to dig into the indent code that much ...

If you could get pgindent smarter in this area, it would be really
nice..
--
Michael

  4. Re: Emacs vs pg_indent's weird indentation for function declarations

    Thomas Munro <thomas.munro@enterprisedb.com> — 2019-01-28T08:47:14Z

    On Mon, Jan 28, 2019 at 8:08 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, Jan 28, 2019 at 12:28:30AM -0500, Tom Lane wrote:
> > Thomas Munro <thomas.munro@enterprisedb.com> writes:
> >> That's ... annoying.  I wish indent wouldn't do that, because it means
> >> that my declarations get moved around every time I write code.
> >
> > If you can fix it, I'd vote for accepting the patch.  I don't personally
> > have the desire to dig into the indent code that much ...
>
> If you could get pgindent smarter in this area, it would be really
> nice..

Ah, it's not indent doing it, it's pgindent's post_indent subroutine
trying to correct the effects of the (implied) -psl option, but not
doing a complete job of it (it should adjust the indentation lines of
later lines if it changes the first line).

One idea I had was to tell indent not to do that by using -npsl when
processing headers, like in the attached.  That fixes all the headers
I looked at, though of course it doesn't fix the static function
declarations that appear in .c files, so it's not quite the right
answer.

-- 
Thomas Munro
http://www.enterprisedb.com

  5. Re: Emacs vs pg_indent's weird indentation for function declarations

    Thomas Munro <thomas.munro@gmail.com> — 2019-04-07T23:36:25Z

    On Mon, Jan 28, 2019 at 9:48 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Mon, Jan 28, 2019 at 8:08 PM Michael Paquier <michael@paquier.xyz> wrote:
> > If you could get pgindent smarter in this area, it would be really
> > nice..
>
> Ah, it's not indent doing it, it's pgindent's post_indent subroutine
> trying to correct the effects of the (implied) -psl option, but not
> doing a complete job of it (it should adjust the indentation lines of
> later lines if it changes the first line).
>
> One idea I had was to tell indent not to do that by using -npsl when
> processing headers, like in the attached.  That fixes all the headers
> I looked at, though of course it doesn't fix the static function
> declarations that appear in .c files, so it's not quite the right
> answer.

I tried teaching pgindent's post_indent subroutine to unmangle the
multi-line declarations it mangles.  That produces correct
indentation!  But can also produce lines that exceed the column limit
we would normally wrap at (of course, because pg_bsd_indent had less
whitespace on the left when it made wrapping decisions).  Doh.
Attached for posterity, but it's useless.

So I think pg_bsd_indent itself needs to be fixed.  I think I know
where the problem is.  lexi.c isn't looking far ahead enough to
recognise multi-line function declarations:

        if (*buf_ptr == '(' && state->tos <= 1 && state->ind_level == 0 &&
            state->in_parameter_declaration == 0 && state->block_init == 0) {
            char *tp = buf_ptr;
            while (tp < buf_end)
                if (*tp++ == ')' && (*tp == ';' || *tp == ','))
                    goto not_proc;
            strncpy(state->procname, token, sizeof state->procname - 1);
            if (state->in_decl)
                state->in_parameter_declaration = 1;
            return (funcname);
    not_proc:;
        }

That loop that looks for ')' followed by ';' is what causes the lexer
to conclude that the "foo" is an "ident" rather than a "funcname",
given the following input:

  extern void foo(int i);

But if because buf_end ends at the newline, it can't see the
semi-colon and concludes that "foo" is a "funcname" here:

  extern void foo(int i,
                  int j);

That determination causes indent.c's procnames_start_line (-psl) mode
to put "extern void" on its own line here and stop thinking of it as a
declaration:

    case funcname:
    case ident:     /* got an identifier or constant */
        if (ps.in_decl) {
            if (type_code == funcname) {
                ps.in_decl = false;
                if (procnames_start_line && s_code != e_code) {
                    *e_code = '\0';
                    dump_line();
                }

I guess it'd need something smarter than fill_buffer() to see far
enough, but simply loading N lines at once wouldn't be enough because
you could still happen to be looking at the final line in the buffer;
you'd probably need a sliding window.  I'm not planning on trying to
fix that myself in the short term, but since it annoys me every time I
commit anything, I couldn't resist figuring out where it's coming from
at least...

-- 
Thomas Munro
https://enterprisedb.com

  6. Re: Emacs vs pg_indent's weird indentation for function declarations

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-15T04:43:10Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> I tried teaching pgindent's post_indent subroutine to unmangle the
> multi-line declarations it mangles.  That produces correct
> indentation!  But can also produce lines that exceed the column limit
> we would normally wrap at (of course, because pg_bsd_indent had less
> whitespace on the left when it made wrapping decisions).  Doh.
> Attached for posterity, but it's useless.

> So I think pg_bsd_indent itself needs to be fixed.  I think I know
> where the problem is.  lexi.c isn't looking far ahead enough to
> recognise multi-line function declarations:

I experimented with fixing this.  I was able to get pg_bsd_indent to
distinguish multi-line function declarations from definitions, but it
turns out that it doesn't help your concern about the lines being too
long after re-indenting.  Contrary to what you imagine above, it seems
pg_bsd_indent will not reflow argument lists, regardless of whether it
thinks there needs to be more or less leading whitespace.  I'm a bit
surprised that -bc doesn't cause that to happen, but it doesn't (and I'm
not sure we'd really want to force one-parameter-per-line, anyway).

Anyway, the attached hasty-and-undercommented change to pg_bsd_indent
allows removal of the "Move prototype names to the same line as return
type" hack in pgindent, and we then get prototypes with properly
lined-up arguments, but we'll have a lot of places with over-length
lines needing manual fixing.  Unless somebody wants to find where to
fix that in pg_bsd_indent, but I've had my fill of looking at that
spaghetti code for today.

			regards, tom lane

  7. Re: Emacs vs pg_indent's weird indentation for function declarations

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-15T21:17:01Z

    I wrote:
> I experimented with fixing this.  I was able to get pg_bsd_indent to
> distinguish multi-line function declarations from definitions, but it
> turns out that it doesn't help your concern about the lines being too
> long after re-indenting.  Contrary to what you imagine above, it seems
> pg_bsd_indent will not reflow argument lists, regardless of whether it
> thinks there needs to be more or less leading whitespace.

Actually, now that I think about it, pgindent seldom revisits line-break
decisions in code anyway; it's only aggressive about reflowing comments.
So maybe we shouldn't be expecting it to fix this.

Also, looking at sample results (attached), it seems like we don't have
such a large problem as one might guess.  It looks like people have
mostly formatted declarations to work with this indentation already,
either because their editors did it automatically, or because they were
thinking that this might get fixed someday.  (I know I've often had that
in the back of my mind.)  There are some places in the attached diff
that I might take the trouble to clean up manually, but not that many.

I found out that my initial draft of a multi-line lookahead function was
not nearly smart enough to survive contact with the PG source corpus,
but after rejiggering it to cope with comments and attributes, it seems
to do quite well.

A small problem with the "rejiggering" is that it now makes the wrong
choice for K&R-style function definitions, causing them to be weirdly
indented.  For our purposes, that's a non-problem so I'm not excited
about trying to make it smart enough to recognize those.  We do have
a couple of amazingly old and crufty K&R-style functions in src/port/,
though, so probably we'd wish to fix those.

Attached is working draft of pg_bsd_indent changes (still sans comments)
as well as a patch showing the difference between current pgindent
results on HEAD and the results of this version.  I think there's no
question that this is an improvement.

			regards, tom lane

  8. Re: Emacs vs pg_indent's weird indentation for function declarations

    Thomas Munro <thomas.munro@gmail.com> — 2019-05-15T21:26:47Z

    On Thu, May 16, 2019 at 9:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > I experimented with fixing this.  I was able to get pg_bsd_indent to
> > distinguish multi-line function declarations from definitions, but it
> > turns out that it doesn't help your concern about the lines being too
> > long after re-indenting.  Contrary to what you imagine above, it seems
> > pg_bsd_indent will not reflow argument lists, regardless of whether it
> > thinks there needs to be more or less leading whitespace.
>
> Actually, now that I think about it, pgindent seldom revisits line-break
> decisions in code anyway; it's only aggressive about reflowing comments.
> So maybe we shouldn't be expecting it to fix this.

Ahh, so my quick and dirty perl change was probably OK then.  Sorry I
didn't realise that before, but this certainly a better way, fixing
the right thing.

> A small problem with the "rejiggering" is that it now makes the wrong
> choice for K&R-style function definitions, causing them to be weirdly
> indented.  For our purposes, that's a non-problem so I'm not excited
> about trying to make it smart enough to recognize those.  We do have
> a couple of amazingly old and crufty K&R-style functions in src/port/,
> though, so probably we'd wish to fix those.

What kid of fork is pg_bsd_indent... do we care about upstreaming
changes?  I guess someone would need to deal with that case eventually
if so.

> Attached is working draft of pg_bsd_indent changes (still sans comments)
> as well as a patch showing the difference between current pgindent
> results on HEAD and the results of this version.  I think there's no
> question that this is an improvement.

+1

Thanks for looking into it!

-- 
Thomas Munro
https://enterprisedb.com

  9. Re: Emacs vs pg_indent's weird indentation for function declarations

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-15T21:30:42Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, May 16, 2019 at 9:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A small problem with the "rejiggering" is that it now makes the wrong
>> choice for K&R-style function definitions, causing them to be weirdly
>> indented.  For our purposes, that's a non-problem so I'm not excited
>> about trying to make it smart enough to recognize those.  We do have
>> a couple of amazingly old and crufty K&R-style functions in src/port/,
>> though, so probably we'd wish to fix those.

> What kid of fork is pg_bsd_indent... do we care about upstreaming
> changes?  I guess someone would need to deal with that case eventually
> if so.

We regard the FreeBSD copy as upstream, and I think we're mostly in sync
with that but only mostly.  So it would come down to whether the FreeBSD
maintainer is worried about K&R mode and what he wants to do about that.
Piotr, that's still you isn't it?

			regards, tom lane

  10. Re: Emacs vs pg_indent's weird indentation for function declarations

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-17T14:48:03Z

    I wrote:
>>> A small problem with the "rejiggering" is that it now makes the wrong
>>> choice for K&R-style function definitions, causing them to be weirdly
>>> indented.  For our purposes, that's a non-problem so I'm not excited
>>> about trying to make it smart enough to recognize those.  We do have
>>> a couple of amazingly old and crufty K&R-style functions in src/port/,
>>> though, so probably we'd wish to fix those.

To be concrete about that: the existing behavior when trying to decide
whether "foo(" starts a function declaration or a function definition
is to scan to the matching right paren[1] and then, if the immediately
next character is ';' or ',', it's a declaration; else it's a definition.
This fails on decls with attributes, such as

extern void pg_re_throw(void) __attribute__((noreturn));

My patch changes the rule to be "scan until we see a ';' ',' or '{'
that's not within parens; then it's a definition if that character
is '{', otherwise a declaration".  So it gets the right answer
for the above, but the wrong answer for

int
isinf(x)
double		x;
{

It doesn't really seem practical to me to make the lookahead function
smart enough to tell the difference between attributes and K&R-style
parameter declarations.  What I'm thinking of doing to have an
upstreamable patch is to invent a new switch, perhaps '-kr'/'-nkr',
to indicate whether the user is more worried about K&R function
declarations than she is about function attributes.  The default
for this switch could be debated perhaps, but I'd just stick an
explicit -nkr into pgindent's invocation, so we wouldn't care too
much.  (This would also ensure that pgindent would fail if you
try to use a too-old copy of pg_bsd_indent...)

			regards, tom lane

[1] Um, well, actually the existing behavior is to scan to the
*first* right paren.  But upgrading it to count parens correctly
seems like an unobjectionable improvement in any case.

  11. Re: Emacs vs pg_indent's weird indentation for function declarations

    Piotr Stefaniak <postgres@piotr-stefaniak.me> — 2019-05-19T17:25:05Z

    On 17/05/2019 16.48, Tom Lane wrote:
> I wrote:
>>>> A small problem with the "rejiggering" is that it now makes the wrong
>>>> choice for K&R-style function definitions, causing them to be weirdly
>>>> indented.  For our purposes, that's a non-problem so I'm not excited
>>>> about trying to make it smart enough to recognize those.  We do have
>>>> a couple of amazingly old and crufty K&R-style functions in src/port/,
>>>> though, so probably we'd wish to fix those.

> It doesn't really seem practical to me to make the lookahead function
> smart enough to tell the difference between attributes and K&R-style
> parameter declarations.  What I'm thinking of doing to have an
> upstreamable patch is to invent a new switch, perhaps '-kr'/'-nkr',
> to indicate whether the user is more worried about K&R function
> declarations than she is about function attributes. 

I think it's safe to assume that upstream can drop support for K&R-style 
parameters altogether.

  12. Re: Emacs vs pg_indent's weird indentation for function declarations

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-19T17:27:21Z

    Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> On 17/05/2019 16.48, Tom Lane wrote:
>> It doesn't really seem practical to me to make the lookahead function
>> smart enough to tell the difference between attributes and K&R-style
>> parameter declarations.  What I'm thinking of doing to have an
>> upstreamable patch is to invent a new switch, perhaps '-kr'/'-nkr',
>> to indicate whether the user is more worried about K&R function
>> declarations than she is about function attributes. 

> I think it's safe to assume that upstream can drop support for K&R-style 
> parameters altogether.

Cool.  I already created the switch, but maybe we could have it
default to -nkr?

			regards, tom lane

  13. Re: Emacs vs pg_indent's weird indentation for function declarations

    Piotr Stefaniak <postgres@piotr-stefaniak.me> — 2019-05-19T17:50:50Z

    On 19/05/2019 19.27, Tom Lane wrote:
> Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
>> On 17/05/2019 16.48, Tom Lane wrote:
>>> It doesn't really seem practical to me to make the lookahead function
>>> smart enough to tell the difference between attributes and K&R-style
>>> parameter declarations.  What I'm thinking of doing to have an
>>> upstreamable patch is to invent a new switch, perhaps '-kr'/'-nkr',
>>> to indicate whether the user is more worried about K&R function
>>> declarations than she is about function attributes.
> 
>> I think it's safe to assume that upstream can drop support for K&R-style
>> parameters altogether.
> 
> Cool.  I already created the switch, but maybe we could have it
> default to -nkr?

Sorry, but GNU indent already uses -kr for something else and I would 
like FreeBSD indent have something like that under the same name. 
Besides, indent has too many options and this one doesn't look like 
particularly desired by anyone. It's possible that someone will complain 
some day, but I don't think we should assume that they'll do or that 
they're more important than the other users who benefit from your change 
being the default behavior and no additional options.

  14. Re: Emacs vs pg_indent's weird indentation for function declarations

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-19T18:13:09Z

    Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> On 19/05/2019 19.27, Tom Lane wrote:
>> Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
>>> I think it's safe to assume that upstream can drop support for K&R-style
>>> parameters altogether.

>> Cool.  I already created the switch, but maybe we could have it
>> default to -nkr?

> Sorry, but GNU indent already uses -kr for something else and I would 
> like FreeBSD indent have something like that under the same name. 
> Besides, indent has too many options and this one doesn't look like 
> particularly desired by anyone. It's possible that someone will complain 
> some day, but I don't think we should assume that they'll do or that 
> they're more important than the other users who benefit from your change 
> being the default behavior and no additional options.

Huh.  OK, I'll rip the switch back out again.

			regards, tom lane

  15. Re: Emacs vs pg_indent's weird indentation for function declarations

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-19T19:48:02Z

    I wrote:
> Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
>> Sorry, but GNU indent already uses -kr for something else and I would 
>> like FreeBSD indent have something like that under the same name. 
>> Besides, indent has too many options and this one doesn't look like 
>> particularly desired by anyone. It's possible that someone will complain 
>> some day, but I don't think we should assume that they'll do or that 
>> they're more important than the other users who benefit from your change 
>> being the default behavior and no additional options.

> Huh.  OK, I'll rip the switch back out again.

Here's a proposed patch for you.

			regards, tom lane