perl5
perl5 copied to clipboard
Perl
Draft: Whitespace Cleanup Manually Maintained Header Files
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.
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)
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 https://github.com/Perl/perl5/pull/20209#issuecomment-1234525184, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2DH6DQ7IG465FRESZSZ3V4DL7JANCNFSM6AAAAAAQCIX2JE. You are receiving this because you are subscribed to this thread.Message ID: @.***>
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/"
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.
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.
I didn't look at per-commit, but at the differences between this and blead.
Overall, this is a big improvement over what we now have, IMO. Here are the exceptions to that I see
It is controversial whether sentences should be separated by 1 vs 2 blanks after the terminating punctuation mark. I don't think therefore it is a good idea to defacto make a single space our standard. Thus I dont agree with the removal of double-spaces in XSUB.h, for example, or in a number of comments throughout.
There are a bunch of places where things are needlessly shifted to the right, so that something that used to fit in 80 columns no longer does. I can't get github to show this property, as it squashes strings of white space, but an example is in cop.h lines 793,795 changing to 802,804.
And there are places with the opposite problem, like lines 733-735 of inline.h where comments were squeezed to the left so that there's only a single space separating them from what they are commenting. That lost visual separation is helpful.
After seeing so many of the above, I gave up and looked at a diff with ignoring all white space changes. It looks like a bunch of this is machine generated, so the algorithm could be improved. We have this change, which makes no sense
< #define ANYOF_UNIPROP ((ANYOF_POSIXL_MAX)+4) /* Used to indicate a Unicode
< property: \p{} or \P{} */
---
> #define ANYOF_UNIPROP ((ANYOF_POSIXL_MAX)+4) \
> /* Used to indicate a Unicode property: \p{} or \P{} */
creating a continuation line just for a comment
In inline.h 2096,2101c2097,2102 suddenly the comments lines are inappropriately indented. vim does that to me sometimes.
`
@khwilliamson thanks for the feedback, ill review in detail later.
ALL: I messed up and fat finger pushed a broken version of the PR, ill fix it up later, possibly as much as week from now as I will be pretty busy the coming week and I dont know how much hacking time ill get. Any feedback though I will follow up on.
On Thu, 1 Sept 2022, 18: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:
- lines 11-40 are misaligned
- lines 111-167: comments that continue on the next line are no longer aligned
- perl.h:
- lines 5223-5555
- 5556-5599
- 5605-5636
- 5642-5681
- 5842-5849
- utfebcdic.h:
- lines 30-32
Yeah, I found some bugs that I think would explain this. I'll review.
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)
Not just you. I pushed that because I sometimes notice issues there I don't in a local diff. I think I fixed that locally. I'll repush at some point.
Please let me know any other examples that annoy you. Likely I will want to fix them. But perhaps wait my next push to see what fixes I have queued up already.
Yves
After thinking about this some more, I don't think we should be adjusting white space in comments other than to wrap text and expand any tabs.
I do have a perl function that does that in a WIP workspace that I could polish up.
After thinking about this some more, I don't think we should be adjusting white space in comments other than to wrap text and expand any tabs.
@khwilliamson at first I thought the same, but over time names have been mass adjusted, text changed, etc, with sometimes unfortunate results. I dont mind excluding "block comments" from my code, as for the stuff i really care about (making the code and comments readable) mostly concerns "hanging comments".
But FWIW, I have taken the time to write my own text reflow around Text::Wrap, (i am working with damian conway to make Text::Autoformat better with C comments but for now i have switched away from it). As part of that i have hacked support for "two spaces after period". and some other stuff. ..
I just did a push with the latest state, and it seems to be much improved these days over how it looked in earlier days. Maybe take a check?
BTW, id love to hear why you think we should "just leave it alone". Is there a specific reason? What about when the comment is over 80 columns because something was renamed with a search and replace? Wouldnt it be better to have a consistent format as much as possible?
My thought was that once the output of this was stable and more or less agreed on, id make it into a Porting tool. Then when people like you or me (or whomever) make mass changes that might mess things up we can just run the script. Then if you add a new define the tool will realign for you, or whatever.
I said that I don't object to wrapping.
The problem I see in adjusting comment blanks in general is that you don't know what the author was doing that actually was planned, like creating a table or other construct where things in adjacent rows line up vertically for clarity or to make things easier to read.
Ok, so only touch a block comment if it actually contains a line that is longer than desired?
Anyway, for what its worth i put some effort into making it avoid tables. Also anything in a comment that "verbatim style" is left alone. A block of whitespace indented text inside of a large comment will be left alone also. I was thinking if we made this a permanent thing we could also have some special notation to say "this leave this please".
On 10/28/22 08:50, Yves Orton wrote:
Ok, so only touch a block comment if it actually contains a line that is longer than desired?
Your proposal below likely stops my concerns
Anyway, for what its worth i put some effort into making it avoid tables. Also anything in a comment that "verbatim style" is left alone. A block of whitespace indented text inside of a large comment will be left alone also. I was thinking if we made this a permanent thing we could also have some special notation to say "this leave this please".
This may be good enough
I finally started to review this closely, but I want to compare it with blead, and it needs to be rebased. I should now have some time so if @demerphq rebases it, I will look at it soon thereafter.
@khwilliamson I dropped this for a bit. Ill get it regenerated onto the latest version of blead ASAP.
Oh, hrm, that chain of reflow C comments at the end means something is wrong. I will check what it is.
With the merger of https://github.com/Perl/perl5/pull/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.
On Fri, 30 Dec 2022 at 22:14, James E Keenan @.***> wrote:
With the merger of #20442 https://github.com/Perl/perl5/pull/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