Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix indentation #48

Open
229c9cf0 opened this issue Aug 24, 2019 · 8 comments · May be fixed by #58
Open

fix indentation #48

229c9cf0 opened this issue Aug 24, 2019 · 8 comments · May be fixed by #58

Comments

@229c9cf0
Copy link

Indentation is completely broken right now. There's a weird mix of tabs and spaces, indentation depths differs, … It's bad enough that gcc is confused and (wrongly) complains about unguarded statements all over the place:

src/m_input.c:622:6: warning: this ‘else’ clause does not guard... [-Wmisleading-indentation]
src/d_code.c:357:2: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/d_code.c:414:2: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/m_maths.c:232:2: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/m_maths.c:284:2: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/m_maths.c:310:2: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/i_display.c:3536:6: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/i_display.c:4015:7: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/i_display.c:4298:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/i_display.c:7282:6: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/i_display.c:13572:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/t_template.c:188:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/v_interp.c:1740:1: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/h_story.c:1279:11: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/h_story.c:1284:11: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/g_method_std.c:273:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/e_files.c:596:2: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/v_draw_panel.c:376:1: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_fix.c:540:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_fix.c:940:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/g_game.c:503:2: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/g_world_map_2.c:1675:6: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/t_files.c:195:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_compile.c:1305:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_compile.c:1367:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_compile.c:1425:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_compile.c:1464:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_compile.c:1536:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_compile.c:1631:2: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_compile.c:1638:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
src/c_compile.c:1659:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]

It's also really hard to read…

Could you fix this?

Alternatively, I could try to set up and run a code formatter on this and make a pull request – in which case, what should the indentation be?

  • Tabs or spaces? Currently, there's a pretty even mix of both… (I personally prefer tabs, as everyone can just set the tab width in their editor and then have whatever indentation depth they prefer. But spaces are also fine, just make it tabs XOR spaces.)
  • If using spaces, how many spaces per indentation level? Lots of places use one character, some two, and then there's also larger gaps (3, 5, 9?!, …) when looking at the output of a quick grep|sed|sort|uniq chain. (For me, single-space indentation is hard to read – with spaces, I'd suggest at least two per indentation level.)
@melvinzhang
Copy link
Contributor

melvinzhang commented Aug 25, 2019

I've tried clang-format on the code base and it works, the trick is to set {SortIncludes: false} as rearranging includes would break compilation. I've checked the generated assembly code on Linux and it is identical after formatting. I can generate some examples of using clang-format with different styles for comparison.

See this gist for c_init.c in different clang-format styles:
https://gist.github.com/melvinzhang/def98589d85da808fe6034c6bad6a249

@linleyh
Copy link
Owner

linleyh commented Aug 25, 2019

I won't try to defend any aspect of my terrible code formatting. Anything that makes it less terrible sounds good.

@oglinuk
Copy link
Contributor

oglinuk commented Dec 27, 2021

I would be happy to go through and clean up the indentation of files, but would like to ask if you prefer tabs or spaces @linleyh?

@linleyh
Copy link
Owner

linleyh commented Dec 27, 2021

Great!
Is there a more standard/popular approach to indentation these days? I'm happy either way.

@oglinuk
Copy link
Contributor

oglinuk commented Dec 27, 2021

I am unsure what others use (other than @melvinzhang suggested clang style), but I try to follow the Linux kernel development coding style. They use 8 character indentation, which they state "Rationale: The whole idea behind indentation is to clearly define where a block of control starts and ends. Especially when you’ve been looking at your screen for 20 straight hours, you’ll find it a lot easier to see how the indentation works if you have large indentations." Would like to get thoughts though.

@linleyh
Copy link
Owner

linleyh commented Dec 30, 2021

If that's how the Linux kernel is indented, it's probably the way to go.

@melvinzhang
Copy link
Contributor

https://github.com/torvalds/linux/blob/master/.clang-format can be used to produce linux kernel style using clang-format. Unfortunately, clang-format is missing support for alignment of defines and initializers, see https://www.kernel.org/doc/html/latest/process/clang-format.html#missing-support

@linleyh
Copy link
Owner

linleyh commented Dec 31, 2021

That should be okay - the code doesn't really have enough defines for that to be a problem, and I don't think it has any designated initialisers.

@oglinuk oglinuk linked a pull request Jan 1, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants