Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format cpp and hpp files in Fw/Types #2677

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Conversation

bocchino
Copy link
Collaborator

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.

};
}

EightyCharString(const char* src); //!< char* source constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
}

EightyCharString(const char* src); //!< char* source constructor
EightyCharString(const StringBase& src); //!< other string constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.

const char* toChar() const; //!< gets char buffer
NATIVE_UINT_TYPE getCapacity() const; //!< return buffer size
InternalInterfaceString(const char* src); //!< char* source constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
const char* toChar() const; //!< gets char buffer
NATIVE_UINT_TYPE getCapacity() const; //!< return buffer size
InternalInterfaceString(const char* src); //!< char* source constructor
InternalInterfaceString(const StringBase& src); //!< other string constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.

private:
MemAllocator(MemAllocator&); //!< disable
MemAllocator(MemAllocator*); //!< disable

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
PolyType(); //!< default constructor
PolyType(const PolyType &original); //!< copy constructor
virtual ~PolyType(); //!< destructor
PolyType(F32 val); //!< F32 constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
bool isF32(); //!< F32 checker
PolyType& operator=(F32 val); //!< F32 operator=

PolyType(bool val); //!< bool constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
bool isBool(); //!< bool checker
PolyType& operator=(bool val); //!< bool operator=

PolyType(void* val); //!< void* constructor.

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
};
}

String(const char* src); //!< char* source constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
}

String(const char* src); //!< char* source constructor
String(const StringBase& src); //!< other string constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

} else {
dest = valString;
}
#if FW_SERIALIZABLE_TO_STRING || BUILD_UT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
case TYPE_I8:
(void)snprintf(valString, sizeof(valString), "%" PRId8 " ", this->m_val.i8Val);
break;
#if FW_HAS_16_BIT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
(void)snprintf(valString, sizeof(valString), "%" PRId16 " ", this->m_val.i16Val);
break;
#endif
#if FW_HAS_32_BIT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
(void)snprintf(valString, sizeof(valString), "%" PRId32 " ", this->m_val.i32Val);
break;
#endif
#if FW_HAS_64_BIT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
#endif
#if FW_HAS_F64

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.

#ifdef BUILD_UT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
@Joshua-Anderson
Copy link
Contributor

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
Copy link
Collaborator

thomas-bc commented Apr 15, 2024

Adding CI to check formatting as we start formatting is the plan, tracked by #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
Copy link
Collaborator Author

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
Copy link
Collaborator

Merge it! Merge it!

@LeStarch LeStarch merged commit 9e3fc90 into nasa:devel Apr 15, 2024
33 checks passed
@bocchino bocchino deleted the format-fw-types branch April 25, 2024 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants