search

exasim-project / NeoFOAM

WIP Prototype of a modern CFD core
26 stars 3 forks source link

Enforce a consistent naming scheme #46

Closed MarcelKoch closed 5 months ago

MarcelKoch commented 5 months ago

This PR adds a clang-tidy check to enforce a consistent naming scheme. Currently the scheme is defined as

Other ideas are up to discussion. The clang-tidy check can be adjusted quite a lot, for example different case for const varibles, global variables, abstract classes, etc. The full list can be found here: https://clang.llvm.org/extra/clang-tidy/checks/readability/identifier-naming.html#cmdoption-arg-AbstractClassCase

Right now this check is not enforced. It needs either IDE support (e.g. CLion has this out-of-the-box), or manual invocation. To run it manually do

clang-tidy -p /dir/containing/compile_commands.json FILE [FILES ...]

It requires to have the compile_commands.json available, which can be generated by CMake, by configuring it with -DCMAKE_EXPORT_COMPILE_COMMANDS=ON. IMO we should just always set this option, there is no downside to it.

I will also try to make it part of our git hooks.

MarcelKoch commented 5 months ago

I've added now a GH action to check for clang-tidy warnings.

Using it as part of our git-hooks is not a good idea IMO. clang-tidy requires the path to the compilation database, but determining that when running the git-hooks seems generally not possible. So far, I've only come up with some hacks that create a file which stores the build directory of the last cmake call. But I don't think that is a reasonable solution. The GH action should suffice.

greole commented 5 months ago

In general I prefer snake_case for variables and functions because it makes it easier to see if something is a type or a variable. @HenningScheufler what is your opinion?

greole commented 5 months ago

Ok lets go with camelCase everywhere.

MarcelKoch commented 5 months ago

Does this need to wait on anything? I would suggest applying the fixes in another PR.

HenningScheufler commented 5 months ago

@greole can we merge?