kokkos-kernels icon indicating copy to clipboard operation
kokkos-kernels copied to clipboard

Propose increasing column limit to 120.

Open jgfouca opened this issue 8 months ago • 17 comments

Moving the topic of line length formatting here from https://github.com/kokkos/kokkos-kernels/pull/2247.

I propose extending code format column width from 80 to >100. This may be a bit controversial, but I often feel like our formatter makes my code look worse. A lot of the issue is the line splitting it does makes the code worse to look at. With all the templating we do, it's easy for even simple code to go over 80 characters. I understand that no one wants to see a line wrap on their editor, but I think most of us spend most of our time developing on a nice large monitor, so we would be unlikely to need to line wrap at under 100 characters. As an example, on my current setup, I can have a single source file open with no line wrapping up to 224 characters or 112 if I have two files open side-by-side as I often do.

If the team agrees that we should extend the allowed line length, we need to agree on what length. 120 seemed to get the most agreement when I brought this up at our weekly meeting.

Should we can take this opportunity, while we are modifying our format, to also upgrade to clang-format-16 (or whatever supports C++20) to prep for C++20?

We can also do this all at once in one massive commit or just reformat files as they are touched naturally during KK development.

One concern what that we don't want git blame to show a commit that was just doing formatting changes. Like many git commands, git blame has a -w that will filter whitespace changes. Unfortunately, the -w flag is not smart enough to filter line split changes, so we will need a smarter solution...

Fortunately, one exists! https://git-scm.com/docs/git-blame#Documentation/git-blame.txt---ignore-revltrevgt NOTE: this flag was only added fairly recently; you'll need git version > 2.23 (released in 2021). I tested it and it works great! The downside is that you have to remember to add this flag every time you run git blame. This is where git aliases and a maintained file of formatting commits would be super useful. One person recommended a tracked file, like .gitignore, at the root of the repo. Something like .git-blame-ignore-revs. This would allow us to add formatting/uninteresting commits in the future. We could then all configure our git to make this convenient. The options are:

# Add formatting and other uninteresting commits here and
# 'git blame $file' will skip them IF
# A) you have it configured to do so:
#   git config --global blame.ignoreRevsFile .git-blame-ignore-revs
# OR
# B) you have an aliased blame command for KokkosKernels:
#   git config --global alias.kkblame 'blame --ignore-revs-file=.git-blame-ignore-revs'
#   NOTE: this implies you run 'git kkblame $file'
# OR
# C) you explicitly tell blame to skip them
#  git blame --ignore-revs-file=.git-blame-ignore-revs $file

This --ignore-revs-file approach forces us to do the reformatting in a small number of (maybe even just one) giant commits.

Why do this at all?

Some examples of how much nicer the code looks with a 100 column limit:

-      Kokkos::parallel_for(Kokkos::TeamThreadRange(team, k1, k2),
-                           [&](const size_type k) {
-                             const auto col = Base::U_entries(k);
-                             Base::uset(k, 0.0);
-                             Base::iw(my_team, col) = k;
-                           });
+      Kokkos::parallel_for(Kokkos::TeamThreadRange(team, k1, k2), [&](const size_type k) {
+        const auto col = Base::U_entries(k);
+        Base::uset(k, 0.0);
+        Base::iw(my_team, col) = k;
+      });
-                               KokkosBlas::Algo::Gemv::Unblocked>::invoke(team,
-                                                                          one,
-                                                                          Ljj, Xj,
-                                                                          zero,
+                               KokkosBlas::Algo::Gemv::Unblocked>::invoke(team, one, Ljj, Xj, zero,
-          LowerTriLvlSchedTP1SolverFunctor<RowMapType, EntriesType, ValuesType,
-                                           LHSType, RHSType>
-              tstf(row_map, entries, values, lhs, rhs, nodes_grouped_by_level,
-                   node_count);
+          LowerTriLvlSchedTP1SolverFunctor<RowMapType, EntriesType, ValuesType, LHSType, RHSType>
+              tstf(row_map, entries, values, lhs, rhs, nodes_grouped_by_level, node_count);
-        } else if (thandle.get_algorithm() ==
-                       SPTRSVAlgorithm::SUPERNODAL_SPMV ||
-                   thandle.get_algorithm() ==
-                       SPTRSVAlgorithm::SUPERNODAL_SPMV_DAG) {
+        } else if (thandle.get_algorithm() == SPTRSVAlgorithm::SUPERNODAL_SPMV ||
+                   thandle.get_algorithm() == SPTRSVAlgorithm::SUPERNODAL_SPMV_DAG) {
-    std::cout << " + Execution space   : " << execution_space::name()
-              << std::endl;
+    std::cout << " + Execution space   : " << execution_space::name() << std::endl;
     std::cout << " + Memory space      : " << temp_mem_space::name() << std::endl;
-    std::cout << " + SpTrsv(lower) time: " << sptrsv_time_seconds << std::endl
-              << std::endl;
+    std::cout << " + SpTrsv(lower) time: " << sptrsv_time_seconds << std::endl << std::endl;

I added the entire team as reviewers since the code formatting change would impact everyone.

jgfouca avatar Jun 24 '24 18:06 jgfouca