search

InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.76k stars 945 forks source link

Improve clang-format to check newlines at end of file #2127

Open NeroBurner opened 1 month ago

NeroBurner commented 1 month ago

Verification

Introduce the issue

I regularly criticize missing newline at end of file resulting in stop symbol of GitHub stop symbol of GitHub at end of files

@mark9064 suggested to let clang-format do this check https://github.com/InfiniTimeOrg/InfiniTime/pull/2107#discussion_r1765677368

@FintasticMan adds info on which clang-format version we could use https://github.com/InfiniTimeOrg/InfiniTime/pull/2107#discussion_r1769634701

I've just checked, and the option to insert trailing newlines has been added in clang-format 16. Last time I updated it was 14, so it wasn't included in that. I'll do a upgrade to a newer version soon.

What version do you think is reasonable to update to? Unfortunately, I don't think that clang-format ignores unrecognised options, so it'll be a minimum version for anyone using it.

Preferred solution

clang-format checks the missing newline at end of file in CI

Version

main

FintasticMan commented 1 month ago

The latest version of clang-format is 19, the latest version in the Ubuntu 24.04 repositories (which we use for CI) is 18, and the latest version in the Debian stable repos is 16 (just for a more conservative comparison). I'd be quite happy to go straight for 18, but I really don't know which versions other developers are using.

If anyone is using a version of clang-format below 18, could you let me know here, along with which OS and version? Thanks!

NeroBurner commented 1 month ago

Ubuntu 22.04 has clang-format-14

https://packages.ubuntu.com/jammy/clang-format

Clang-format-16 is available since Ubuntu 23.10 https://packages.ubuntu.com/search?keywords=clang-format-16

Just a data point, although I'm on arch so I have newer packages available

mark9064 commented 1 month ago

In other words, the option we want is supported by Debian stable, Ubuntu LTS and anything newer right? So in that case I think we're safe to enable it

Do we even specify a minimum clang format version anywhere (in code rather than docs)? If so I think it shouldn't be bumped past Debian stable (16) unless we want some newer feature

FintasticMan commented 1 month ago

We do specify a minimum version in the clang-format commit hook, so I'll just bump to 16 for now.