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

Add clang-format checks to CI #17194

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

Conversation

pitbuster
Copy link
Contributor

@pitbuster pitbuster commented Jul 24, 2024

@TurboGit I know this will be some effort, but having consistent format should ease contributions: I was bitten by this, because the project has some formatting rules, which IDEs or language servers will pick, but are not enforced, so a lot of unwanted changes that people have to revert.

I see two options for landing this:

  1. Have the PR open while we (I guess mostly me since I am the one proposing) reformat files bit by bit (one module at a time, for example).
  2. Land this with a reduced number of files to check and we incrementally fix the formatting while we add more files to the whitelist.

Also, probably the formatting rules themselves will cause a bit of discussion, but they can be changed as we reach consensus :)

@wpferguson
Copy link
Member

I'm not sure this is a good thing. There is already a style guide that's followed.

Is the purpose of this to make it work with IDEs? I don't think most of the developers use one, so changing the code style for IDEs would probably hamper development more than it would help.

What would help development is refactoring some of the HUGE files into smaller chunks.

@fcs-ts
Copy link

fcs-ts commented Jul 28, 2024

There is already a style guide that's followed.

No, there is already a format, but it's not being followed, I have had several unwanted formatting changes on previous PRs

so changing the code style for IDEs would probably hamper development more than it would help.

This is not changing anything, just taking the already existing formatting rules and enforcing them on PRs. I am sorry if the description made it sound different.

What would help development is refactoring some of the HUGE files into smaller chunks.

Of course, but automatic formatting enforcement is usually the baseline to make contributions easier :)

@parafin
Copy link
Member

parafin commented Jul 28, 2024

I think you’re missing the point. clang format file is not used by most (all) developers, so it’s not being followed or kept up-to-date. It needs to be either removed completely, or updated with current code style. It is not a ground truth to be enforced.

That being said, I’m not sure if current code style is described anywhere, except for @TurboGit comments on PRs. That should be the first step - formulating it somewhere.

@wpferguson
Copy link
Member

wpferguson commented Jul 28, 2024

I’m not sure if current code style is described anywhere

https://github.com/darktable-org/darktable/wiki/Developer's-guide

@fcs-ts
Copy link

fcs-ts commented Jul 28, 2024

It is not a ground truth to be enforced.

Yes, I thought the code style would resemble more closely the current clang file. I guess I'll ask some help to @TurboGit to bring the file up to date then :)

@parafin
Copy link
Member

parafin commented Jul 28, 2024

I’m not sure if current code style is described anywhere

https://github.com/darktable-org/darktable/wiki/Developer's-guide

That does not describe it fully. For example Pascal asks for function parameters to be one per line.

@pitbuster
Copy link
Contributor Author

I just realized that I used another account to reply to comments (that is, @fcs-ts is me 😅).

I took a look at the .clang-format file and it seems to cover the "function parameters in their own lines rule". I guess there are other rules that have come up in reviews, but I guess we should wait for Pascal to be back to move this forward.

@TurboGit
Copy link
Member

TurboGit commented Aug 7, 2024

@pitbuster : In the style there is at least 3 very important rules to me:

  1. Never ever reformat the SQL code as this was a long and boring manual reformatting work. At least in current state most (if not all) SQL statement are readable.
    DT_DEBUG_SQLITE3_PREPARE_V2(dt_database_get(darktable.db),
                                "INSERT INTO main.color_labels (imgid, color)"
                                "  SELECT ?1, color"
                                "  FROM main.color_labels"
                                "  WHERE imgid = ?2",
                                -1, &stmt, NULL);
  1. A single parameter per line for a function spec/definition.
void commit_params(struct dt_iop_module_t *self,
                   dt_iop_params_t *p1,
                   dt_dev_pixelpipe_t *pipe,
                   dt_dev_pixelpipe_iop_t *piece);
  1. Not more than 90 characters per line

  2. Boolean operator should be at start of expression and one per line

  if(!strcasecmp(cc, ".dt")
     || !strcasecmp(cc, ".dttags")
     || !strcasecmp(cc, ".xmp"))

The rest can be discussed on the way. You're free to gather and document the style guide.

This https://github.com/darktable-org/darktable/wiki/Developer's-guide#coding-style has not been touched since long time. Maybe you can take it over and update the style?

@pitbuster
Copy link
Contributor Author

@TurboGit I updated the section with your feedback (and also fixed the language hints for codeblocks, so we get highlighting in the wiki :D). Next, I'll try to see how far are the current clang-format rules from the ones you stated here :)

@pitbuster
Copy link
Contributor Author

I tweaked the rules a little bit to be closer to the points mentioned in the discussion. I also formatted src/bauhaus/bauhaus.h as a demo, so we can see if the rules look fine.

Copy link

This pull request has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please verify it has no conflicts with the master branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants