nasa / fprime

F´ - A flight software and embedded systems framework
https://fprime.jpl.nasa.gov
Apache License 2.0
9.96k stars 1.29k forks source link

Format cpp and hpp files in Fw/Types #2677

Closed bocchino closed 3 months ago

bocchino commented 3 months ago

Reformatting improves the readability of the code. If we do it module by module, the impact should be minimal. Fw/Types seems like a logical place to start.

If someone is developing Fw/Types with the old formatting, that person just needs to reformat the code being developed before diffing or making a PR against the new formatting. The catch is that because of the idiosyncratic use of the preprocessor symbols PRIVATE and PROTECTED in F Prime, you can't just run clang-format. Here is the script that I use to work around this issue:

#!/bin/sh -e

# ----------------------------------------------------------------------
# fprime-format-cpp
# Use clang-format to format C++ files for F Prime
# ----------------------------------------------------------------------

format()
{
  sed -e 's;^[ \t]*PROTECTED[ \t]*:;protected: // fprime-format-cpp;' -e 's;^[ \t]*PRIVATE[ \t]*:;private: // fprime-format-cpp;' | \
  clang-format --style=file | \
  sed -e 's;protected: *// fprime-format-cpp;PROTECTED:;' -e 's;private: *// fprime-format-cpp;PRIVATE:;'
}

if test $# -eq 0
then
  format
else
  for file in "$@"
  do
    cp "$file" "$file.bak"
    format < "$file.bak" > "$file"
  done
fi

I call this tool fprime-format-cpp. When run anywhere in the F Prime repo, it picks up the formatting configuration stored in [fprime-root]/.clang-format, so the formatting should be consistent for all users. It's designed so that you can run it inside a text editor (e.g., vi or VS Code) or in batch mode on the command line.

Here is how you can reformat all the files in a build module:

% fprime-format-cpp `find . -name '*.hpp' -or -name '*.cpp'`

I recommend that we put fprime-format-cpp, or something like it, into the repo so users can use it to format their code.

Joshua-Anderson commented 3 months ago

This is awesome!

I wish we could find an alternative to our idiosyncratic use of PRIVATE/PUBLIC, but I think solving that is a longer term issue and shouldn't hold up code formatting. This script is a great workaround.

Only feedback is I'd highly recommend setting up CI to check formatting of Fw/Types to prevent regressions going forward. Otherwise by the time you finish applying formatting everywhere other code changes will probably have regressed formatting on previously formatted code.

thomas-bc commented 3 months ago

Adding CI to check formatting as we start formatting is the plan, tracked by https://github.com/nasa/fprime/issues/1984

Also, note that we have fprime-util format which does the formatting with clang-format and the same mechanism you used in your bash script. It can take in a file list through

fprime-util format -f Main.cpp Main.hpp

or with stdin

find . | fprime-util format --stdin
bocchino commented 3 months ago

Also, note that we have fprime-util format which does the formatting with clang-format and the same mechanism you used in your bash script.

Great!

LeStarch commented 3 months ago

Merge it! Merge it!