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

JANITORIAL: run clang-format on all files (DO NOT MERGE) #1776

Closed
wants to merge 1 commit into from

Conversation

@Mataniko
Copy link
Contributor

Mataniko commented Jul 27, 2019

Using this PR to start a conversation. Do we want to use a tool like clang-format to automate a coding standard?

There's obviously more work to do like adding a bunch of scripts and tweaking the rules, but this should give everyone a general idea of how it would look.

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Jul 27, 2019

I will take a closer look into the actual result yet, but in answering your question on automate coding standard.

Yes, we historically were keeping up-to-date config for astyle: https://wiki.scummvm.org/index.php?title=Code_Formatting_Conventions#Automatically_converting_code_to_our_conventions

So, this is great to have yet-another-tool, especially if it produces a better result.

However, if somebody wants to put it into some fully automated way, that would be not good, as every now and then, the code authors want some specific code formatting.

Tooling, however, could help tremendously in mass-fixing lousy student work, or when we import the original sources.

@lephilousophe

This comment has been minimized.

Copy link
Member

lephilousophe commented Jul 27, 2019

Thanks for these new rules. astyle is a little limited about what it formats.
I looked at how it transformed cryomni3d engine code because I applied astyle to it when I submitted it.
There is some formatting I am not fond of.

  • The member initializer list gets split at each member with commas at next line. As I understand why commas are there, I don't really like to see my list split in multiple lines. I think it slows down reading before getting to the core of the constructor. (I would remove BreakConstructorInitializers)
  • Many lines get joined. That lines were split for a reason (grouping of conditions for example) and CLang isn't able to determine that. Maybe it shouldn't join back lines. (it seems to need some formatting added to the code)
  • Empty for loops get their closing brace moved to the next line. I think it adds unnecessary lines to read. (AllowShortLoopsOnASingleLine maybe?)
  • Pure virtual functions sometimes get their = 0 moved to next line. I didn't find why clang did this.
  • In macros, concatenation operator ## gets attached to operands unlike other operators.
  • Nested namespaces seem to indent functions declared in them. I am not sure it's desirable. (NamespaceIndentation could be set to None).
  • Alignment of consecutive enums and defines is broken. (AlignConsecutiveMacros et al could be set to true

I think I covered much of what I saw.
What do you think?

@athrxx

This comment has been minimized.

Copy link
Member

athrxx commented Jul 27, 2019

While having a tool like that is a good thing I'd prefer if it weren't run over the whole code base. As sev pointed out it will remove all sorts of intentional formatting. That certainly applies to some files I've just looked at which were written by me.

If the general feeling is that such a tool HAS to be run on the whole code base, I'd prefer it to be a 'light' version that sticks to things like tabs/white space. Although even that can have unpleasant results, e. g. static data block which have been formatted into rectangle shapes will lose that shape:

Maybe the PR could be limited to code portions that really seem to be in need of it...

@Mataniko

This comment has been minimized.

Copy link
Contributor Author

Mataniko commented Jul 27, 2019

I agree with everything that's been said. The purpose of the PR is not to merge a full codebase style, but to give the most complete preview of how things look, where to improve and whether there is an appetite getting this integrated into the repo.

Some of the rules I've put in place to match the existing code base, like BreakConstructorInitializers, others could use some tweaking to have the most desirable results.

The biggest difficulty in applying something like this on our codebase is identifying where the formatting was intentional. clang-format does allow you to disable formatting using comment blocks // clang-format off // clang-format on but there's too many places to find.

Once I get some more feedback, I'll adjust rules and see what parts of the code base this makes sense for smaller PRs

@athrxx

This comment has been minimized.

Copy link
Member

athrxx commented Jul 27, 2019

Thanks for that clarification and for the work you put into this.
So, regarding BreakConstructorInitializers, is something like that actually intended (especially the leading commas):

TownsAudioInterfaceInternal::TownsAudioInterfaceInternal(Audio::Mixer *mixer, TownsAudioInterface *owner, TownsAudioInterfacePluginDriver *driver, bool externalMutexHandling)
: TownsPC98_FmSynth(mixer, kTypeTowns, externalMutexHandling)
, _fmInstruments(0)
, _pcmInstruments(0)
, _pcmChan(0)
, _waveTables(0)
, _waveTablesTotalDataSize(0)
, _baserate(55125.0f / (float)mixer->getOutputRate())
, _tickLength(0)

Maybe that rule could be dropped. Or do people actually find that pleasant? Let's find out :-)

@dreammaster

This comment has been minimized.

Copy link
Member

dreammaster commented Jul 27, 2019

Maybe that rule could be dropped. Or do people actually find that pleasant? Let's find out :-)

I think it's nasty. I always put the commas at the end of the previous row, since it's clearer in my mind as indicator that "something follows this value". Having it like above on a separate just seems plain weird.

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Jul 28, 2019

I also agree with what the others said. It's nice to have a specific set of coding style rules, but it's hard to apply them to all of the existing code base.

A lot of them are subjective, but we could update the wiki page with the least invasive ones.

Also, we could also consider adding an .editorconfig file in the master branch, so that some simple coding conventions can be applied in all IDEs:

https://editorconfig.org/

@SupSuper

This comment has been minimized.

Copy link
Contributor

SupSuper commented Jul 28, 2019

I'm all for clang-format since it's very powerful and integrates with a lot of IDEs, which astyle does not. However it's important to document the clang-format version everyone's using because different versions will produce different results.

I agree with the rest that automating and enforcing something as big as ScummVM is tricky, so it should be applied on a case-by-case basis.

@Mataniko My suggestions for rule changes are:

  • Set the tab-width's to 4 instead of 2 (per the wiki).
  • NamespaceIndentation: None (we use a lot of nested namespaces in subengines and such). The only case I've seen indented is in forward-declarations, which there isn't a rule for.
  • SpaceAfterTemplateKeyword: false (per the wiki).
  • Standard: Cpp03 (since we support compilers without C++11, this will avoid formatting nested templates like Vector<Vector<int>>).
  • BreakConstructorInitializers: BeforeColon
  • A lot of engines sort "scummvm" includes separately from the "engine" includes, so if we wanna standardize that, might be worth adding it to IncludeCategories.

@bluegr As far as I can tell .editorconfig only enforces tab width so I'm not sure how useful it is.

@Mataniko

This comment has been minimized.

Copy link
Contributor Author

Mataniko commented Jul 28, 2019

Might just have to pack in clang-format for windows, linux and macOS in the devtools or dist folders

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Aug 25, 2019

Is there a reason to keep this open? Should we adjust our code formatting guidelines in the wiki?

@Mataniko

This comment has been minimized.

Copy link
Contributor Author

Mataniko commented Aug 25, 2019

Going to close, I'm going to eventually incorporate some of the feedback and so smaller merges of modules.

@Mataniko Mataniko closed this Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.