Draft: Whitespace Cleanup Manually Maintained Header Files

[demerphq]

This patch sequences cleans up various minor nits in our manually managed header files (eg, *.h in the root of the repo) , and then introduces a whitespace fixup patch to put all our header files into a standardized format, with defines using as consistent formatting as possible, multiline macros definitions having consistently formatted line continuation markers and other things like that. Similarly structs and unions are consistently formatted with the * character from pointer declarations being associated with the name not the type like we would normally for variables, and with the member declarations lined up properly and with comments at consistent positions in the definition. In other words the last patch in this sequence should ONLY make whitespace changes.

I wanted to have this patch so we could start a conversation about whether we should use some kind of tool to enforce a consistent format. (I think we should.) I find the inconsistencies in the formatting makes the header files harder to read, and i think programs are better at it than we are, so at least for header files I thought i would see what people think.

[bram-perl]

I think the first commits can be merged as-is,

For the whitespace cleanup commit, skimming the files:

• op.h doesn't look good:
  • lines 11-40 are misaligned
  • lines 111-167: comments that continue on the next line are no longer aligned
• perl.h:
  • lines 5223-5555 5523-5555
  • 5556-5599
  • 5605-5636
  • 5642-5681
  • 5842-5849
• utfebcdic.h:
  • lines 30-32

Other things I am not fond of:

(Taking a random example)
regexp.h

        struct {
            /* this first element must match u.yes */
            struct regmatch_state   *prev_yes_state;
            U32                     lastparen;
            U32                     lastcloseparen;
            CHECKPOINT              cp;

        }                            branchlike;

The name of the struct is too far to the right (again IMO)

(Update: fix typo in line numbers of perl.h)

[khwilliamson]

On 9/1/22 10:40, Bram wrote: I think the first commits can be merged as-is, For the whitespace cleanup commit, skimming the files: * op.h doesn't look good: o lines 11-40 are misaligned o lines 111-167: comments that continue on the next line are no longer aligned * perl.h: o lines 5223-5555 o 5556-5599 o 5605-5636 o 5642-5681 o 5842-5849 * utfebcdic.h: o lines 30-32 Other things *I* am not fond of: (Taking a random example) regexp.h |struct { /* this first element must match u.yes */ struct regmatch_state *prev_yes_state; U32 lastparen; U32 lastcloseparen; CHECKPOINT cp; } branchlike; | The name of the struct is too far to the right (again IMO)

agreed

…

— Reply to this email directly, view it on GitHub <#20209 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA2DH6DQ7IG465FRESZSZ3V4DL7JANCNFSM6AAAAAAQCIX2JE>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[demerphq]

On Thu, 1 Sept 2022 at 20:42, Karl Williamson ***@***.***> wrote:

On 9/1/22 10:40, Bram wrote: > I think the first commits can be merged as-is, > > For the whitespace cleanup commit, skimming the files: > > * op.h doesn't look good: > o lines 11-40 are misaligned > o lines 111-167: comments that continue on the next line are no > longer aligned > * perl.h: > o lines 5223-5555 > o 5556-5599 > o 5605-5636 > o 5642-5681 > o 5842-5849 > * utfebcdic.h: > o lines 30-32

I think I understand what caused these, I may have fixed some, but i think not all, or maybe not any, will fix later today.

> The name of the struct is too far to the right (again IMO) agreed

That should be fixed in the latest push. Which is not perfect in a number a ways that I am aware of already. ;-) Particularly "fixing" the indent on multiline STMT_START macros. Feel free to review but if it regards the latter point I'm already aware and on it. Yves

…

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[demerphq]

I have pushed a significant revamp of the white space cleanups. This also includes sundry changes to infra that is whitespace sensitive or sensitive to line continuation markers in our header files.

I dont think this is "good enough" to merge yet, but i do think its "good enough" to hear folks comments. One issue for me is lining up things can lead to comments straying past the 80 column boundary. I havent decided the right thing to do there, and some of the choices the code makes may not be ideal.

[bram-perl]

Preamble:

• I reviewed commit-per-commit;
• Some commits I merely skimmed and didn't fully read
• I know some of the changes that I left comments on was changed again in a later commit (basically resolving the comment); I still left them in place tho just in case they might be useful for other cases.

[bram-perl]

commit XS-APItest/t/locale.t - deal with indented values properly

It would be nice to amend the commit message with an example of what failed to parse
and/or what parsed incorrectly..

Also: I think it fixes more then just indented values?
I'm assuming (not verified) that before this commit something like: #define FOO "A B C" would get parsed incorrectly

[demerphq]

I updated the commit message. I suspect you are right, but the code in practice doesn't have such things. It was just that if you changed a define from define foo bar into define foo__ bar (assume the underbars are spaces here) the parse code would see the define name as foo__ (that is 2 spaces) and then fail. Such white-space sensitivity is unhelpful. :-) And as you said, had it had a more complex value it would have gotten things worse.

[bram-perl]

commit (3) break multiline define

Not entirely sure of these..
I might be tempted to indent the ternary in this case

i.e. something like:

#define dXSTARG                                                     \
    SV * const targ = ((PL_op->op_private & OPpENTERSUB_HASTARG)    \
        ? PAD_SV(PL_op->op_targ) : sv_newmortal())

(but I suppose it might be out of scope for this comment)

[demerphq]

I can try to come up with some heuristics, but there is a mixture of cases, some of which expect things to line up precisely, some which could do with an indent like you suggest. If i can figure out a more or less reliable heuristic to distinguish them I am game, but so far anything i tried like that seemed to help a small handful of cases and make the rest worse. This isn't done by hand obviously. :-)

[bram-perl]

commit (3) break multiline define

I may also have indented in this case but maybe with just one space because the first ( is not yet closed.
So something like:

#  define CopFILEAVx(c)     \
       (assert_(CopFILE(c)) \
        GvAV(gv_fetchfile(CopFILE(c))))

or

#  define CopFILEAVx(c)     \
       (assert_(CopFILE(c)) \
           GvAV(gv_fetchfile(CopFILE(c))))

[demerphq]

Same comment as to your previous feedback. If I can figure out a way to tell when it is something like this:

#define X (foo
          |bar
          |baz)

from cases where the indent would make sense then I will. In general however i found preserving the existing line up as much as possible has produced the widest range of reasonable results.

[bram-perl]

commit (3) break multiline define

Here too I would indent

[demerphq]

Or collapse the parens entirely maybe. I can try something with this, its a special enough case it shouldnt cause much problems (compared to the previous two points).

[demerphq]

I hacked late last night, and i see that the latest push has some broken artifacts. Shoulda packed it in earlier. Just a FYI I know about it and im fixing.

[demerphq]

Oh, hrm, that chain of reflow C comments at the end means something is wrong. I will check what it is.

[demerphq]

and fixed.

[jkeenan]

With the merger of #20442 into blead today, is this pull request still needed? If so, merge conflicts will need to be resolved and the p.r.'s status will have to get out of Draft.

[demerphq]

On Fri, 30 Dec 2022 at 22:14, James E Keenan ***@***.***> wrote: With the merger of #20442 <#20442> into blead today, is this pull request still needed? If so, merge conflicts will need to be resolved and the p.r.'s status will have to get out of Draft.

I'll get this rebased. FWIW, yes, it is still a work in progress or even just a discussion starter, which is why it is stalled in draft status. A nice chunk of it goes away with #20442 however. Yves

[bulk88]

I would appreciate the tool. Less thinking/time for everyone, what the "right way" is for the p5p repo.