-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Cpp: Remove code duplication #3995
Conversation
23f2e50
to
60ab5b5
Compare
A note on the unity build enabling: Using unity builds can cause the build to fail, even though it worked locally. The reason for such failures is then symbol duplication caused by two source files having defined the exact same symbols. If such source files get mangled together into a single unity build file, then that'll create a compiler error. I still think unity builds are tremendously useful, but I can also remove the commit that enables them, if you consider unity builds to cause a too convoluted build. |
@hzeller any experience with unity builds? |
I have no experience with unity builds, but I can see how that can mess up things.
|
The same function was defined inside multiple different source files (in an anonymous namespace). Not only is this generally bad practice from a maintenance point of view, but it also break unity builds (which can speed up compilation times A LOT). The fix simply consists in introducing a new Utils file that contains the single implementation and every source file that uses this function simply has to be linked against that new file (and include the corresponding header). Signed-off-by: Robert Adam <dev@robert-adam.de>
This should reduce the build time for those steps significantly Note: Unity builds are enabled only for cmake-based CI builds Signed-off-by: Robert Adam <dev@robert-adam.de>
abe483e
to
bf19d2c
Compare
I applied the requested changes and rebased against |
I think I would keep the unity build because it will help detect these cases. Maybe something like
|
On second thought, it might be worthwhile having both builds: with and without unity. So either
|
Signed-off-by: Robert Adam <dev@robert-adam.de>
Indeed. I adapted the workflow file accordingly 👍 |
I think this looks good. @parrt , handing over to you. |
Thanks! |
* Cpp: Remove code duplication The same function was defined inside multiple different source files (in an anonymous namespace). Not only is this generally bad practice from a maintenance point of view, but it also break unity builds (which can speed up compilation times A LOT). The fix simply consists in introducing a new Utils file that contains the single implementation and every source file that uses this function simply has to be linked against that new file (and include the corresponding header). Signed-off-by: Robert Adam <dev@robert-adam.de> * CI: Enable unity builds for cpp targets This should reduce the build time for those steps significantly Note: Unity builds are enabled only for cmake-based CI builds Signed-off-by: Robert Adam <dev@robert-adam.de> * CI: Perform unity and non-unity cpp builds Signed-off-by: Robert Adam <dev@robert-adam.de> --------- Signed-off-by: Robert Adam <dev@robert-adam.de> Signed-off-by: Jim.Idle <jimi@idle.ws>
The same function was defined inside multiple different source files (in an anonymous namespace).
Not only is this generally bad practice from a maintenance point of view, but it also break unity builds (which can speed up compilation times A LOT).
The fix simply consists in introducing a new
HashUtils
file that contains the single implementation and every source file that uses this function simply has to be linked against that new file (and include the corresponding header).