Skip to content

treewide: run clang-format - #2035

Closed
nschonni wants to merge 2 commits into
coredevices:mainfrom
nschonni:clang-format
Closed

treewide: run clang-format#2035
nschonni wants to merge 2 commits into
coredevices:mainfrom
nschonni:clang-format

Conversation

@nschonni

@nschonni nschonni commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Was curious after changing a minor one, but it didn't really uncover much.

nschonni
nschonni commented Sep 7, 2026
View reviewed changes
Comment thread tools/applib_malloc.template.c Outdated
nschonni
nschonni commented Sep 7, 2026
View reviewed changes
Comment thread .github/workflows/compliance.yml Outdated
@nschonni
nschonni marked this pull request as draft September 7, 2026 17:56
@nschonni

nschonni commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Oops, this didn't use normal globs, so the CI job is picking up a lot more.
Running find . -iname '*.h' -o -iname '*.c' | xargs clang-format -i changes way to many files, so closing

@nschonni nschonni closed this Sep 7, 2026
@nschonni
nschonni deleted the clang-format branch September 7, 2026 18:24
gmarull
gmarull reviewed Sep 7, 2026
View reviewed changes

@gmarull gmarull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm all in for clang-format, we just need to find the right settings and do it in a massive treewide + enforce from there on

@nschonni
nschonni restored the clang-format branch September 7, 2026 21:57
@nschonni nschonni reopened this Sep 7, 2026
@nschonni
nschonni force-pushed the clang-format branch 5 times, most recently from 573f0f6 to 22f6bb6 Compare September 7, 2026 23:28
@nschonni

nschonni commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@gmarull I realized I was using v14, so I grabbed the latest and generated a new config with clang-format-22 -style=Google -dump-config include/bluetooth/analytics.h > .clang-format. I Reset a few of the values so they mactched the few already in the file, then used find . -iname '*.h' -o -iname '*.c' | xargs clang-format-22 -i to apply across the repo.
I'm guessing it might be better for you to take this over as a new PR, but maybe the .clang-format and the updated CI job might help

gmarull
gmarull reviewed Sep 8, 2026
View reviewed changes
Comment thread soc/nrf/nrf52/sleep.c Outdated
gmarull
gmarull reviewed Sep 8, 2026
View reviewed changes
Comment thread src/fw/apps/sdk/app.c Outdated
Comment on lines +131 to +137
.common = {.main_func = s_main,
// UUID: 1197fc39-47e7-439b-82be-f56d9ba1dbd8
.uuid = {0x11, 0x97, 0xfc, 0x39, 0x47, 0xe7, 0x43, 0x9b, 0x82, 0xbe, 0xf5, 0x6d,
0x9b, 0xa1, 0xdb, 0xd8},
.process_type = ProcessTypeWatchface},
.icon_resource_id = RESOURCE_ID_MENU_ICON_TICTOC_WATCH,
.name = "TicToc"};

@gmarull gmarull Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd prefer line breaks, so we have more columns available at the cost of vertical space (ie no {.name = ...)

@nschonni nschonni Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Think I covered this with the last push that added the linebreaks in lists, but that doesn't cover all the spacing stuff

gmarull
gmarull reviewed Sep 8, 2026
View reviewed changes
Comment thread src/fw/apps/system/alarms/alarm_editor.h Outdated
@nschonni
nschonni force-pushed the clang-format branch 6 times, most recently from c6a0f3f to 45ee50a Compare September 14, 2026 21:27
@nschonni

nschonni commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

The failure for test_kraepelin_algorithm seems to reoccur after a few rebases, but I'm not sure the root issue. I'm guessing one of the format string/template changes is causing it

@gmarull

gmarull commented Sep 15, 2026

Copy link
Copy Markdown
Member

can you create a single commit with clang-format changes, then a 2nd that reformats? (we'll probably take the first, merge, then reformat)

@nschonni

nschonni commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Sure, I just kept the other commits to make the review of what the setting change easier to roll off, if the change was making it worse. I figured you might want to take this over anyway, but happy to iterate a bit more if you want to point out any of the resulting code that still looks off.
I guess I should see if I can trace down the test failure. I did have to add .clang-format-ignore to exclude a few files that were failing the build before even getting to the test.

@nschonni
nschonni force-pushed the clang-format branch 2 times, most recently from 7291350 to d44e748 Compare September 15, 2026 22:17
@nschonni

nschonni commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

@gmarull I split/squashed stuff. Did you also want me to move the CI part to it's own commit?

@nschonni
nschonni force-pushed the clang-format branch 2 times, most recently from dec80cd to 0f88eeb Compare September 16, 2026 03:10
@nschonni
treewide: update clang-format config with CI
2cd49cf 
Signed-off-by: Nick Schonning <nschonni@gmail.com>
@nschonni
treewide: apply clang-format
6c9904a 
Signed-off-by: Nick Schonning <nschonni@gmail.com>
@nschonni
nschonni marked this pull request as ready for review September 16, 2026 03:34
@nschonni

nschonni commented Sep 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I ignored the tests/ folder so CI is green again
Failing test run can be seen on the previous runs like https://github.com/coredevices/PebbleOS/actions/runs/35050119885/job/104648492825?pr=2035

@gmarull

gmarull commented Sep 16, 2026

Copy link
Copy Markdown
Member

+1 from my side, @jplexer

@jplexer

jplexer commented Sep 16, 2026

Copy link
Copy Markdown
Member

i guess gotta rebase again (lmao) but sure lets do it

@gmarull gmarull mentioned this pull request Sep 16, 2026
Merged
@nschonni

nschonni commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favour of #2085

@nschonni nschonni closed this Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmarull gmarull gmarull left review comments
@jplexer jplexer Awaiting requested review from jplexer jplexer is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nschonni @gmarull @jplexer