Indentation (tabs or spaces)

[JohannesLorenz]

Looking at the current code base, indentation is made using tabs, which also conforms to our coding conventions. The conventions, however, don't name the length for each tab. You can usually check line breaks or constructor initializer lists to see what tab length was used in the past.

• src/core/Instrument.cpp: 4
• plugins/LadspaEffect/LadspaSubPluginFeatures.h: 4
• src/core/ProjectRenderer.cpp: 8
• plugins/GigPlayer/GigPlayer.cpp: 8

The most files seem to use 4, though.

Note: We can do automatic conversions using #4690 .

Please vote (multiple votes allowed):

• 🎉 Use tab lenght of 4
• ❤️ Use tab length of 8
• 🚀 Change tabs into spaces (length 4)
• 👎 Other option, specify

[JohannesLorenz]

I'm for staying with tabs, and use a length of 4:

• this seems to be used in the most files
• this also allows longer code lines then a length of 8 (especially if you have many if/switch/for etc).

[zonkmachine]

The conventions, however, don't name the length for each tab.

I believe the convention used to specify a tab length of 8.

[JohannesLorenz]

Note: Github by default uses a tab size of 8 when you display a file in the web browser. We can fix this adding an .editorconfig to LMMS, like suggested on SO.

[PhysSong]

There is another way, indenting by tabs and aligning by spaces. It's a bit complicated, but one can change the tab size without breaking alignments.

[Spekular]

I assume tabs + spaces for alignment is what's meant by the tab options.

[ghost]

Are we still going to use .clang-format? (#4690)

To be honest, I don't care whether it's tabs or spaces. I work a lot in upstream libraries and whatever you choose, I'll still going to have to edit my options to match the tabs/space flavour of that lib. I used to have an opinion but now I'm more leaning towards: "f it! let a formatter figure it out!"

Which bring us back to .clang-format. :-)

PS: I vote in favor for #4690 (whatever the settings are)

[enp2s0]

8-space tabs.

C++ is hard to read with small indents because of braces etc. So I prefer a width of 8.
I know most people don't agree with me, so I prefer tabs because then everyone can set whatever width they want.

That being said, align with spaces. Tabs are for indents and indents only, that way people can change tab size and not screw up alignment throughout the project.

[tresf]

I strongly prefer spaces for layout consistency but if the product is largely maintained by people using editors that don't detect spaces and switch automatically it would be a burden on those developers.

What I believe is most important is that the main contributors to the codebase (e.g. past 3 years) come to a decision that works for them. When I say main contributors, I mean those that are actually typing large amounts of code (as opposed to people like myself that usually patch a few lines of the build system).

A vote is nice, but this isn't a democracy. Those that code the most get to decide what the project switches to.

[tresf]

Are we still going to use .clang-format? (#4690)

Yes.

[darkmastermindz]

I think 2 or 4 spaces is good enough and for tabs, people can set that to inserting "n amount of spaces" instead of tabs for the project for consistency. I think it's easier to detect and keep track of spaces than tabs extraneous tabs.

[JohannesLorenz]

@tresf You said that the main coders should vote. Should I look into the git log and tag the relevant coders?

[enp2s0]

@darkmastermindz CPP is almost unreadable at 2 space tabs imo.

[JohannesLorenz]

Coders of the last 1.5 years with >=10 commits (run on master, but should include merged branches):

$ git shortlog -s HEAD~200..HEAD | sort -n | grep '^[[:space:]]*[0-9]\{2\}'
    14  DomClark
    16  Dominic Clark
    16  Michael Gregorius
    19  Hussam Eddin Alhomsi
    21  Javier Serrano Polo
    24  Colin Wallace
    35  Johannes Lorenz
    35  Oskar Wallgren
    40  Hyunin Song
    72  Tres Finocchiaro
    82  Hyunjin Song
   165  Lukas W

I can't see all votes anymore, so can you all please vote (again), commenting on this post? Sorry for the mentions on advance.

• @DomClark
• @michaelgregorius
• @Sawuare
• @jasp00
• @Wallacoloo
• @zonkmachine
• @PhysSong
• @tresf
• @lukas-w

Reminder:

• 🎉 Use tab lenght of 4
• ❤️ Use tab length of 8
• 🚀 Change tabs into spaces (length 4)
• 👎 Other option, specify

[Wallacoloo]

It really doesn’t matter so long as we have a linter or clang-tidy or something that runs as part of the PR process. I’m all for any solution that just makes things consistent across the whole codebase.

If some person is ready to fix up all the indentation to be consistent and add some linter/clang-tidy/whatever, let them do so using the rules of their preference. Effectively I’m in favor of “whoever does the work to achieve consistency gets to decide what the conventions ought to be”. Once you have some tool in place to enforce these conventions, it should become fairly trivial to change them after the fact anyway, so it’s not critical that you choose the “perfect” convention from the start.

[tresf]

it’s not critical that you choose the “perfect” convention from the start

No, but it's courteous to ask though :)

[SecondFlight]

I have a couple PRs with >10 commits each and almost 2000 LOC, but they're not merged as I'm putting it off till 1.2 is out. Any objections if I vote as well?

[SecondFlight]

For reference:
#4369
#4367

[JohannesLorenz]

Sorry, I mean, everyone feeling as a "current coder" can/should vote. This was just to get the majority in.

[lukas-w]

As long as there no hard feelings about the current way, I strongly believe it's best to stick to what we use at the moment, which is tabs for indentation and spaces for alignment. If done correctly, tab length doesn't matter.

Changing all code to use spaces for indentation will touch almost every single line in our codebase. This will inevitably get us into a merge conflict hell, which won't exactly help our 60+ open PRs. As long as we don't have the people to handle all these and their potential merge conflicts, I'm concerned that with such an enormous change we'll be shooting ourselves in the foot.

[tresf]

@lukas-w I don't agree. I feel any auto-formatting will kill the PRs regardless. I don't think the PRs can be saved persay. What's more important, I feel, is to have the big PRs merged prior to the style.

For example, most files us { on dedicated lines and ( space ) and 80-char wraps. The indentation is only going to screw things up slightly more than what would happen otherwise. I strongly feel that it's going to be a mess regardless and that the spacing should not be the sticking point.

[JohannesLorenz]

Shouldn't auto-formatting both the feature and target branches lead to no merge issues? I.e. if you want to merge your branch and discover the target branch being formatted, just format your branch and then merge?

[enp2s0]

I don't know if this will affect the project in any way, but this will destroy git blame and assign every line to whoever ran the auto-format, right?

[tresf]

@JohannesLorenz probably worth testing out that theory. The issue with large commits isn't so much the differences but rather the timing of them. In my experience git often can't handle the timing of the changes properly even on small commits if the same lines are touched at different times. A test on one of the larger PRs would tell us for sure.

[michaelgregorius]

I voted "Change tabs into spaces (length 4)". It's what we use at work and the advantage is that it looks the same everywhere.

[Sawuare]

For what it's worth:
main.cpp is approx. 24 kB.
main.cpp with tab converted to 4 spaces is approx. 29 kB.

[lukas-w]

What's more important, I feel, is to have the big PRs merged prior to the style.

Yes that would be best, I'm arguing that this is what we lack resources for though.

I strongly feel that it's going to be a mess regardless

In this case, frankly I'd prefer not doing any auto-formatting at all, also because of the point @noahb01 made about git blame. I frequently use git blame to understand code by looking at its history. Formatting a codebase as large as LMMS's will certainly be disruptive. I'd personally value branches, PRs and a searchable git history quite a bit higher than having consistent whitespace. I'm not very active on this project anymore though, so I won't stand in the way if the current developers think a more consistent formatting is worth it.

[JohannesLorenz]

git blame can ignore whitespace and code moved into the next line:

git blame -w -M

For indentation -w is sufficient. So as long as every coder/tester/... is aware of -w, there are not problems at all 😄 . If they don't, ... 😞

If anyone wants to change their vote based on that fact, go ahead.

---

@tresf I regularly changed more than whitespace on two branches and merged them together, which always worked, I never got merge conflicts on those lines. What do you mean git "can't handle"?

[tresf]

When git.does a rebase (requires for merging some PRs) it always struggles with modified lines, even minor modifications.

If whitespace isn't a concern, great. In my experience it struggles -- even with with minor modifications -- when doing a rebase because it needs to rewrite history on a feature branch and aligning changes is messy. I'd rather not talk about it though. It can be tested and then there's no need for explaining.

[PhysSong]

@JohannesLorenz Do you still want to keep this open?

[JohannesLorenz]

@JohannesLorenz Do you still want to keep this open?

I'll close this, as we have agreed on one style (4 space-width tabs) and use it in the clang-format PR ( #4690 ).

Thanks for the reminder.