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

Use clang-format to format the whole codebase #278

Merged
merged 8 commits into from
Feb 27, 2024
Merged

Use clang-format to format the whole codebase #278

merged 8 commits into from
Feb 27, 2024

Conversation

stv0g
Copy link
Contributor

@stv0g stv0g commented Feb 26, 2024

Following up on the clang-format discussion, I now have drafted a PR to reformat the whole code base using the clang-format tool with its LLVM preset. Please have a look at the LLVM coding standards for a description of this style.

This PR does:

  • Reformat the entire code base using clang-format
  • Add a .clang-format configuration file for the project
  • Add a .git-blame-ignore-revs file which hides the reformatting commit from git blame and GitHubs blame web-view
  • Add a scripts/format-all.sh script to quickly reformat all C++ source and headers files of the project.
  • Extends devcontainer.json to suggest installing a couple of extensions which are useful for DPsim-related C++ & Python development
    • This includes an extensions for clang-format to format files on save
  • Fix a few missing includes which broke compilation after the reformatting

This PR does NOT:

  • Enforce the use of clang-format
  • Performs any checks if the code is formatted

The idea I have is that we are tolerant towards violate the LLVM style for any commits as we want to keep the barrier for new contributors as low as possible. We dont want to drive them away because of failing nit-pick CI checks.

Hence, we only reformat the entire code-base from time to time by hand.

And of course:

I invite all regular contributors to install the clang-format VSCode extension, so your changes are directly properly formatted when a file is saved.

(Btw. this also speeds up our own coding a lot as you will not have to worry ever again about code formatting. Just write as you like, save the file and everything is tidy :D)

Signed-off-by: Steffen Vogel <[email protected]>
@stv0g stv0g self-assigned this Feb 26, 2024
@stv0g stv0g marked this pull request as draft February 26, 2024 11:48
Signed-off-by: Steffen Vogel <[email protected]>
…previous mass changes of clang-format

Signed-off-by: Steffen Vogel <[email protected]>
This results in a popup suggesting the installation of these extensions when the Devcontainer is opened in VS code.

Signed-off-by: Steffen Vogel <[email protected]>
@stv0g stv0g marked this pull request as ready for review February 26, 2024 12:45
Copy link

sonarcloud bot commented Feb 26, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@stv0g
Copy link
Contributor Author

stv0g commented Feb 26, 2024

I think we will need to ignore the SonarCloud check in this instance as its merely a false positive.
As the reformatting is touching almost all the code, it treats these changes as new code and mandates higher code quality standards.

And the Windows build appears to be broken for other PRs as well?

@m-mirz
Copy link
Contributor

m-mirz commented Feb 26, 2024

@stv0g it looks fine except for the Windows problem. The CI on the master does not seem to have this error related to spdlog: https://github.com/sogno-platform/dpsim/actions/runs/8049125560/job/21982389736?pr=278#step:3:867

@leonardocarreras
Copy link
Collaborator

leonardocarreras commented Feb 26, 2024

Well, looks like @stv0g would probably be right, the last master was compiled with Current runner version: '2.311.0' while this one from the current PR is with Current runner version: '2.313.0' ... Among other things, cmake and lots of VS components were updated between those two versions...

Looks like we need to update (finally) spdlog or add more warning supressing declarations in the CmakeList

dpsim/CMakeLists.txt

Lines 55 to 73 in e0ae9d9

if(MSVC)
# Silence Visual Studio deprecation warnings
add_definitions(-D_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS)
add_definitions(-D_SILENCE_EXPERIMENTAL_FILESYSTEM_DEPRECATION_WARNING)
# Set exception handling for portability
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /EHsc")
check_cxx_compiler_flag("/W4 /WX" CXX_SUPPORTS_WERROR)
if(CXX_SUPPORTS_WERROR)
# TODO: activate this again after fixing warnings
# set(DPSIM_CXX_FLAGS /W4 /WX)
endif()
else()
check_cxx_compiler_flag("-Wall -Werror" CXX_SUPPORTS_WERROR)
if(CXX_SUPPORTS_WERROR)
set(DPSIM_CXX_FLAGS -Wall -Werror)
endif()
endif()

Apparently are the following... define _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING or _SILENCE_ALL_MS_EXT_DEPRECATION_WARNINGS as described in the step mentioned by @m-mirz

@stv0g it looks fine except for the Windows problem. The CI on the master does not seem to have this error related to spdlog: https://github.com/sogno-platform/dpsim/actions/runs/8049125560/job/21982389736?pr=278#step:3:867

I guess that is something to be solved in a different issue and merged before this PR, or not?

For Sonar, I agree that the code needs to be fixed at a different PR, not here.

@m-mirz
Copy link
Contributor

m-mirz commented Feb 27, 2024

@leonardocarreras @stv0g If the Windows build is broken anyway, we can also merge this PR first and solve this issue in a separate PR later ;-)

@m-mirz m-mirz merged commit b17546a into master Feb 27, 2024
19 of 21 checks passed
@stv0g stv0g deleted the clang-format branch February 27, 2024 15:52
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