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

Clang format #126

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

simonschmeisser
Copy link

I experimented a bit with adding clang-format to this repo. The code is unfortunately formated quite creatively so I was not able to decide what is likely the desired formatting. Please feel invited to comment as much as you like and I'll try to improve the settings.

Or do you have an internal clang format file already?

I choose clang-format-12 as that should be available on both Ubuntu 20.04 and 22.04 but if that is not what you use at Jolla I can also propose another version.

I added pre-commit.com which can be configured to run before any commit conveniently. This way your commits will always be clean.

I then formatted the code with some example settings and added the commit hash to .git-blame-ignore-revs. Some git blame tools understand this and won't show the formatting in blames.

Finally I added github ci to run this online in case contributors do not locally.

sudo apt install clang-format-12
pip install pre-commit
pre-commit install

run for all files with

pre-commit run -a

or just have it complain & format once you git commit

Copy link

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be generally positive on reformatting the code with more consistent styling. Even if this is not qt code, we've been roughly following those conventions. E.g. indent of 4, no hard tabs etc. Style here mostly good, but noticed some cases where old code was nicer.

What I'm not sure of is having hooks on clang as in my experience that sort of tooling might be too easily complaining on code details that are perfectly fine, like forcing specific line wrapping etc. Elsewhere we've cleaned up messes by running a formatting tool just once and after that people honoring the conventions better. We could start by just doing the same thing here.

Also now the commits are having some changes a bit back and forth.

AsyncCodecSource.cpp Show resolved Hide resolved
: mReachedEOS(false),
mReading(false) {
}
AsyncCodecSource::Output::Output() : mReachedEOS(false), mReading(false) { }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer here the init list separate and wouldn't like folding the {} body to the line.

if (m_framesReceived.buffers.size() < DROID_MEDIA_CODEC_MAX_INPUT_BUFFERS) {
m_framesReceived.buffers.push_back(buffer);
m_framesReceived.cond.signal();
m_framesReceived.lock.unlock();

return;
}
// either get() will signal us or we will be signaled in case of an error
// either get() will signal us or we will be signaled in case of an
// error

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like relatively short line length being forced here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I had comment reflow enabled together with a 80 char column limit, fixed, will look for other places

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we converged on the parameters to use I will rerun the formatting and create a fresh set of commits not containing this

allocator.cpp Outdated Show resolved Hide resolved
void noteStopCamera(int uid) { }
void noteResetCamera() { }
void noteResetFlashlight() { }
void noteStartSensor(int uid, int sensor)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep empty functions on one line?

@simonschmeisser
Copy link
Author

simonschmeisser commented Feb 20, 2024

Tools like clang format work best if you run them automatically on each commit. Then you never again have to worry or discuss about code formatting and can concentrate on the content. As long as you stick to a certain clang-format version the results will be stable, ie no unrelated diff in any future diff even though you run this on each and every file on each commit.

You mean to check if the rules where applied? Hooks can be run on the developer side to check if code follows the rules a shown e.g. in:
https://invent.kde.org/frameworks/extra-cmake-modules/-/blob/master/kde-modules/kde-git-commit-hooks/clang-format.sh

I picked clang-format-12 here as it is available on Ubuntu 20.04 and 22.04 but if you say people at jolla use 22.04 or similar newish distros clang-format-14 would also be an option

I think Clang 12 is to old if it does limit us.
The current stable Clang version is 17, 12 is to far away I think.

@simonschmeisser
Copy link
Author

I found there is actually a clang-format file for Qt with BSD license and applied it. Need to clean up the history ...

@Thaodan
Copy link
Contributor

Thaodan commented Mar 1, 2024

I found there is actually a clang-format file for Qt with BSD license and applied it. Need to clean up the history ...

The KDE Clangformat file has been quite useful.
It also follows the Qt coding style almost exactly.
This project doesn't use cmake but for any that does the ecm can add hooks when running cmake.

In my project below I'm using those hooks.
They tell me to format the code before commiting.
It's as easy running ninja/make -C build clang-format

@Thaodan
Copy link
Contributor

Thaodan commented Mar 1, 2024

What I'm not sure of is having hooks on clang as in my experience that sort of tooling might be too easily complaining on code details that are perfectly fine, like forcing specific line wrapping etc. Elsewhere we've cleaned up messes by running a formatting tool just once and after that people honoring the conventions better. We could start by just doing the same thing here.

Clang-format configuration can be adjusted per project or optionally per project type e.g. Qt projects.

As the formatting in specific sections can be disable hooks should be always on, unless the commiter tells git to not do so, without it easy for new contributors to miss following the formatting rules.

You add disable formatting for specific section by inserting:

// clang-format off
Some fragile or from third parties imported code...
// clang-format on

as shown below:
https://community.kde.org/Policies/Frameworks_Coding_Style#Disabling_formatting_for_specific_parts

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 this pull request may close these issues.

3 participants