Fix code style corruption introduced by ddmd conversion

[9rnsr]

This is the last PR for my code style frustration. magicport was great tool, but also it was not perfect.

Points:

• Resurrect line spaces. In C-dmd code they had been used for the separator of small code blocks. If it doesn't exist, we need to find the block gaps before understanding the code. It will become huge cost for the code reading repetition.
• Resurrect line wrappings. Too long line is difficult to read.
• Fix indent level in switch statement blocks. The scope statement being immediately below case label had confused magicport conversion. And, current indent level style is not consistent with glue & backend code.
• Place one import declaration per line. This style is diff-friendly when imports will be added/removed.

[9rnsr]

Most of line space positions are same with the c-source files in last-cdmd and v2.068.2 tags.

[9rnsr]

Ping: @WalterBright @yebblies @MartinNowak

[dnadlinger]

Can we please just use dfmt (if somebody is unhappy with its output, then go fix it)? Formatting source code manually is a relic from the last century.

[dnadlinger]

(this obviously does not concern the added line spaces)

[9rnsr]

@klickverbot dfmt does not fit my purpose.

[dnadlinger]

@9rnsr: Why not? Imho there is no excuse for telling everybody to manually format code without a very good reason.

[9rnsr]

(this obviously does not concern the added line spaces)

You meant about other changes than line space additions? If so:

1. I'd like to resurrect structural formatting for complex conditions (eg. in if statement). dfmt cannot do that.
2. Fixing indent level in switch, and import declaration style fix were just trivial work, and using grep + editor key macro was enough for that.

[dnadlinger]

I'd like to resurrect structural formatting for complex conditions (eg. in if statement). dfmt cannot do that.

If you find yourself having an if condition that is so complex you need to format it specially for readability, you should probably refactor it anyway.

I'm not claiming that dfmt is perfect, it definitely could use some more work. However, having used clang-format extensively when working on a large C++ codebase, I'm very convinced that just being able to format code nicely on the press of an editor shortcut helps productivity (and more than I would have expected at first).

[9rnsr]

Of course refactoring is better than just tweaking code format, but it needs modifying runnable code and introduce some risk.

Anyway, I'd just resurrect code readability that was in c-dmd source code. So dfmt does not fit the purpose. Thanks.

[MartinNowak]

Atm there a few things that dfmt doesn't do (for whatever reason people want aligned comments).
#5217

[MartinNowak]

Most of this looks good and improves readability, but I think we'll have to discuss the switch/case changes. My editor and I align them by default, dfmt does as well. What do you mean by scope statement in switch case?

[9rnsr]

My editor and I align them by default, dfmt does as well.

dfmt default setting is not related. From C++ code age, case label is indented inside switch block in dmd source code. Rather I'd like to question about this. Why we've changed style while switching to ddmd? Was there discussion about that? (I don't know about it) If there's not proper decision, I think to keep the historical style is much better than current scrambled indents.

What do you mean by scope statement in switch case?

In C++, a case label don't make a scope implicitly. So to declare some variables under the case label, following style had been used.

case exp1:
{    // <-- scope statement which has same indent with the case label.
    Some variable = declared;
    break;
}
case exp2:

(In D, a case label makes scope implicitly, so later we can remove redundant scope statements. But it should go separated refactoring PR.)

[9rnsr]

If there is no other objections, I'll merge this soon.

[yebblies]

Here's another objection.

[9rnsr]

@yebblies What is "your objection"? Please tell me.

[yebblies]

Many of these changes are just imposing your preferred style on the code, including introducing yet more deviations from what dfmt outputs. Readability comes from consistency, not from preferring one arbitrary style over another.

Fix indent level in switch statement blocks. The scope statement being immediately below case label had confused magicport conversion. And, current indent level style is not consistent with glue & backend code.

There is no point in trying to match the glue/backend style. They don't match the frontend in plenty of other ways.

dfmt default setting is not related. From C++ code age, case label is indented inside switch block in dmd source code. Rather I'd like to question about this. Why we've changed style while switching to ddmd? Was there discussion about that? (I don't know about it) If there's not proper decision, I think to keep the historical style is much better than current scrambled indents.

There was plenty of time before the ddmd switch to review the generate code style. Years.

I'm in favor of line break/indentation changes only if they match dfmt's output. I do support the re-introduction of blank lines, assuming they were deleted by the conversion.

If there is no other objections, I'll merge this soon.

We have the "don't merge your own pull requests" rule for a reason. If you can't convince at least one other contributor...

[9rnsr]

I think dfmt is not yet production use ready (see the output in #5217). Yes, the line wrapping method in this PR is arbitrary, but it's definitely better than the current too long one lines.

Note that, dfmt is not yet support to custom indent level in switch statement (dfmt_align_switch_statements is not yet supported to false).

There is no point in trying to match the glue/backend style. They don't match the frontend in plenty of other ways.

Fix my word. My change matches to the original C++ code style before ddmd conversion.

There was plenty of time before the ddmd switch to review the generate code style. Years.

I didn't have any interests for magicport output, because it was not on my work desk. I've just trusted the generated ddmd.exe behavior. After the ddmd conversion, the broken source code style makes me unhappiness.

I do support the re-introduction of blank lines, assuming they were deleted by the conversion.

magicport had deleted many line spaces, which were for the logical code block separators.

If dfmt is enough ready for production use, I can accept it to use dmd source code. But it's not yet. And I cannot wait coming better dfmt.

If you can't convince at least one other contributor...

I've already answered to the questions of @klickverbot and @MartinNowak . And after that they didn't write any more comments.

[ibuclaw]

I'm in favor of line break/indentation changes only if they match dfmt's output. I do support the re-introduction of blank lines, assuming they were deleted by the conversion.

Can't we just fix dfmt to automatically do what @9rnsr had to do by hand? Just having a quick glance, a few repeated occurrences I see are:

• Imposing a soft/hard line length.
• Blank line after break
• Blank line after braceless if or else body (whichever is the last branch).
• Blank line before comment.

[dnadlinger]

So far this has Daniel's and my veto. Probably not a good idea to spend any more time on piling up even more changes.

Let me also point out that there are much, much worse readability problems with the DMD source code than some white space (cryptic, overly abridged variable names, using random integer constants instead of enums, pretty much no separation of concerns at all because everything changes the public members of everything else, semantic analysis code being arbitrarily put into the glue layer because it is "easier", etc.). I would much prefer if you put your energy into improving those – or at least not making them worse with your other hotfixes.

[9rnsr]

@klickverbot All of things you pointed out have less importance than fixing bugs and regression issues. And at least those have historical reasons. You can open a fixup PR or post a dmd bug.

---

At least to me, the C++ code was better to read. The ddmd conversion was broken dmd code at the readability point. This PR recovers equivalent level readability with C++ code. There's no other things.

To @klickverbot @yebblies : How frequently do you read current ddmd code? Once per month? Once per week? I can believe it would be less than per day, because you're not working on daily dmd bugfixes.

Yesterday, I've posted a bugfix #5271. To do finish it, I've read the related functions at least ten times. It was pain that to read squashed code, and to read over 100 columns lines with horizontal scroll of editor view.

So I can reject your arguments. Only when you really read current broken style code frequently and feel it acceptable, you arguments would have weight.

Again, this is my last handmade fix for the frustrated code style. If we decide to use dfmt in the future, I'll accept it. But it's not today. And I'd continuously work on dmd to reduce bugs for the next release. Please don't block me.

If no other objections, I'll merge this PR in this weekend (11/21 in JST).

[dnadlinger]

If no other objections, I'll merge this PR in this weekend (11/21 in JST).

You are simply not in the position to do so, and neither would I be with my own pull requests.

If you want to spend time on manually adding newlines to the code, then fine, so be it. I'm in favor of those, assuming they are consistent and make the code more readable. I'm also in favor of any other code style fixes, as long as they match what dfmt outputs. Everything else, please don't. We usually reject changes that are aimless churn, and there is no reason why this should be treated differently.

[9rnsr]

@yebblies OK, I removed all two or more spaces in preceding line comment.

[dnadlinger]

@yebblies: Okay to merge?

[andralex]

I am against changes of this kind as a matter of principle. They are labor intensive, massive, trivial, blame a lot of irrelevant changes to one person, and do nothing to improve things. They are an illusion of progress through busywork.

The right answer if such changes are deemed necessary is to improve automated tools such as dfmt.

I'll close this. Please do not reopen, and please let's stay away from this kind of work. Thanks.

[9rnsr]

@andralex Please read discussions. This PR fixes issues which dfmt cannot work for.

[yebblies]

I mostly agree with Andrei, but I don't think that applies to the changes in this PR any more.

And blame is basically dead at this point, so there's not much point worrying about it.

[9rnsr]

@yebblies: So, OK to merge?

[WalterBright]

Some considerations:

1. Do not pull your own PRs. I don't pull mine, and if anyone is entitled to, it should be me.
2. The source as it is has already been released as 2.069. The time for massive reformatting because the code changed anyway in the switch to ddmd has passed.
3. Please respect Andrei's decision to close, despite strongly disagreeing with him. We need to put aside our differences and work together, and that sometimes means accepting things we don't agree with. Many of my PRs have been rejected, too.
4. I dislike single PRs with massive line count changes spread through the project on principle.
5. I am not opposed to doing a bit of reformatting specifically on code that is being fixed anyway. For example, if changing the import list, it would be fine to switch then to one-per-line as part of that PR.

[9rnsr]

1. The source as it is has already been released as 2.069. The time for massive reformatting because the code changed anyway in the switch to ddmd has passed.

I was too late? You said that I was to do is the reformatting before any other bugfixes for the 2.069 release?

1. I dislike single PRs with massive line count changes spread through the project on principle.

In C++ code, I was continuously fixed brace style in each PRs so it's preferred by Andrei to give own lines for them. And some peoples had dislike the incremental fix. That's the reason I opened this big PR in this time.

1. I am not opposed to doing a bit of reformatting specifically on code that is being fixed anyway. For example, if changing the import list, it would be fine to switch then to one-per-line as part of that PR.

OK, I'll separate it to another PR.

[9rnsr]

In really, I don't like to post such the big PR contains no functional changes. However, current squashed code is much harder to read than the previous C++ code. Reading unfolded long conditions is anymore pain.
This reformatting has practical reason to me.

All of line space positions are coming from previous cdmd source code. That's not random.

I'm really question why no other commiters has complaint for the current mechanically generated broken format. Are you really read today's ddmd code? Please answer to me...

[9rnsr]

Now this PR contains only No.1 & 2 changes listed in this comment.

@WalterBright said I should do it before 2.069 release. So I'll stop any other working for the next release.

[yebblies]

@yebblies: So, OK to merge?

Yes, I'm OK with it, but it obviously can't be merged against Walter's veto.

In C++ code, I was continuously fixed brace style in each PRs so it's preferred by Andrei to give own lines for them. And some peoples had dislike the incremental fix. That's the reason I opened this big PR in this time.

Yes, doing it this way is much better.

I'm really question why no other commiters has complaint for the current mechanically generated broken format. Are you really read today's ddmd code? Please answer to me...

I've been reading it for years. It's certainly not idea, and I do agree with many of the style changes you've proposed. But I don't think the readability improvement is worth introducing manual formatting that will hopefully be eventually wiped out by auto-formatting. And at least the current formatting is consistent.

[CyberShadow]

The current incomprehensible mess and technical debt that is the DMD source code, and especially the maintainers' attitude against improvements to the situation as seen in this and previous discussions, is why I will likely never actively contribute to DMD. It is just too depressing.

@andralex With all due respect, you almost never contribute to DMD, so you don't have much first-hand experience of the state of the code base.

[dnadlinger]

@CyberShadow: While I agree with your sentiment (see my comment above), manually indenting switch statements in a different way does nothing to clean up any technical debt. Let's not mistake a discussion about a single questionable and ultimately inconsequential change for some sort of conspiracy against code improvements.

[yebblies]

@WalterBright @andralex I'm not a big fan of big, questionable style changes, but if the alternative is #5303 and #5309 then this is a much better option.

[ibuclaw]

I would have to concur, having reviewed recent PRs, and spending ten minutes working out what exactly has changed in them.