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

support CMAKE_UNITY_BUILD #6104

Closed
wants to merge 1 commit into from

Conversation

tnixeu
Copy link

@tnixeu tnixeu commented Mar 9, 2024

I noticed that cppcheck can not be built with a CMAKE_UNITY_BUILD.

The changes in the PR allow to do a CMAKE_UNITY_BUILD.

Notable changes:

  • move all CWE to the errortypes.h header as extern and the definition to the errortypes.cpp
  • renamed some variables or functions in order to avoid ODR conflicts
  • move code into namespaces in order to avoid ORD conflicts
  • add a hash of the pattern to the generated match and findMatch functions in order to avoid ORD conflicts.

I intentionally tried to keep the initial PR small, so I did not change the indentation for the changed namespaces or do any bigger refactoring.

@firewave
Copy link
Collaborator

firewave commented Mar 9, 2024

Thanks for your contribution.

What do we gain from the unity build? To make sure this doesn't regress we would require to add it to the CI and I would prefer not to have more builds just for the sake of it. Did you also try to build the GUI?

@tnixeu tnixeu force-pushed the support_cmake_unity_build branch from c198f21 to c8a52e9 Compare March 9, 2024 13:12
@tnixeu
Copy link
Author

tnixeu commented Mar 9, 2024

A benefit is that it can lead to faster compile times. Especially for not parallelized builds. It depends on the project.

A problem can be functions and variables in source files with the same name.

I did not check the GUI, I will do this today.

It seems I have messed removed unintentionally a namespace I added before pushing. So I forced pushed in order to fix this.

@tnixeu tnixeu force-pushed the support_cmake_unity_build branch from c8a52e9 to 68a7c4c Compare March 9, 2024 13:45
@tnixeu
Copy link
Author

tnixeu commented Mar 9, 2024

The GUI needed also a few #pragma once and a few renames.

@firewave
Copy link
Collaborator

firewave commented Mar 9, 2024

A benefit is that it can lead to faster compile times. Especially for not parallelized builds. It depends on the project.

IIRC that would only help with build times if you build from scratch (which we never do) as it will produce a single big source file which would also be changed and so it would always build everything.

We also use ccache in the CI to improve build times and this is not compatible with that. So we would have to add an additional build just to make sure that this doesn't break so it would actually take longer until your PRs builds finish. We need less jobs - not more. So I see no point in adding support for this.

Or do you have an actual use case for this?

Also the changes do not improve the code. Some actually do the opposite just to appease this type build. The missing include guards should be added even though they are not of importance. There's also a clang-tidy check for this which we could enable.

@tnixeu
Copy link
Author

tnixeu commented Mar 9, 2024

A benefit is that it can lead to faster compile times. Especially for not parallelized builds. It depends on the project.

IIRC that would only help with build times if you build from scratch (which we never do) as it will produce a single big source file which would also be changed and so it would always build everything.

That depends on the size of the unity build. I usually do unity builds in CI or for full local builds.

We also use ccache in the CI to improve build times and this is not compatible with that. So we would have to add an additional build just to make sure that this doesn't break so it would actually take longer until your PRs builds finish. We need less jobs - not more. So I see no point in adding support for this.

Or do you have an actual use case for this?

Also the changes do not improve the code. Some actually do the opposite just to appease this type build. The missing include guards should be added even though they are not of importance. There's also a clang-tidy check for this which we could enable.

I wanted to speed up a build in an docker image. Other tools I build there I could speed up with a unity build. So I wanted to try to speedup cppcheck as well. However, it does not take as long as the other tools. So it is fine to keep it like this.

@tnixeu tnixeu closed this Mar 9, 2024
@firewave
Copy link
Collaborator

firewave commented Mar 9, 2024

I wanted to speed up a build in an docker image. Other tools I build there I could speed up with a unity build. So I wanted to try to speedup cppcheck as well. However, it does not take as long as the other tools. So it is fine to keep it like this.

If you have a decent CPU (and use Clang) the build is quite fast when you use precompiled headers. GCC is rather slow compared to it.

The build used to be much faster (around 1.7x versions). There is some work in progress to speed things up again. See #4748 and https://trac.cppcheck.net/ticket/11924. I haven't gotten to it yet because of personal reasons as well as encountering more important things to work on

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.

2 participants