kokkos / kokkos-comm

Experimental MPI Wrapper for Kokkos
https://kokkos.org/kokkos-comm/
Other
12 stars 9 forks source link

Modernizing clang-format requirements #46

Closed dssgabriel closed 3 months ago

dssgabriel commented 3 months ago

This project may be the right time to depart from KokkosCore's clang-format-8 requirement and upgrade it to something newer.

I don't know if there are particular (legal?) constraints for such a change, but I suggest we move to at least clang-format-11 (LLVM began supporting -std=c++20 with clang-11). We could go even newer, e.g. with clang-format-14.

Another (personal) suggestion: we should increase the column limit from 80 to 100 (some might even want 120). With current monitor sizes/resolutions, there's no real reason to keep such a narrow width. Moreover, modern C++ can be quite verbose and the 80-column limit causes lots of unnecessary line breaks.

Updating our clang-format while we still have a relatively small codebase is judicious.

dalg24 commented 3 months ago

Please go to at least 14. You might want to survey what popular linux distribution can easily install, like default clang version on the latest LTS of Ubuntu, etc.

dssgabriel commented 3 months ago

I'm fine with going as recent as we can. :slightly_smiling_face:
Ubuntu 24.04 LTS apparently ships LLVM 18! Jammy (22.04) LTS comes with LLVM 14, so we may be fine going with clang-format-14.

Are folks against increasing the column limit to 100? 120?

dutkalex commented 3 months ago

Are folks against increasing the column limit to 100? 120?

I would vote for 120: it's a good tradeoff between compactness and readability for C++ I believe

cwpearson commented 3 months ago

100 or 120 is fine with me.

I was able to brew install llvm@14 on my mac and get a clang-format-14. I think a PR would need 3 pieces:

cwpearson commented 3 months ago

On a linux system I was also able to get clang-format-14 with spack:

spack install llvm@14 -omp_as_runtime
cwpearson commented 3 months ago

https://github.com/kokkos/kokkos-comm/pull/52