cmake: Amend crc32c static library#171
Merged
Merged
Conversation
hebasto
force-pushed
the
240426-cmake-EE
branch
from
d54ad2b to
bea0b36
Compare
Avoid using source-specific compile options. They hard to reason about and error-prone when combining with target-specific compile options. The resulted build logic effectively mirrors the master branch's one.
hebasto
force-pushed
the
240426-cmake-EE
branch
from
bea0b36 to
43fbeac
Compare
theuni
reviewed
Apr 30, 2024
| target_sources(crc32c PRIVATE ${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c_sse42.cc) | ||
| set_property(SOURCE ${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c_sse42.cc | ||
| APPEND PROPERTY COMPILE_OPTIONS ${SSE42_CXXFLAGS} | ||
| add_library(crc32c_sse42 STATIC EXCLUDE_FROM_ALL |
There was a problem hiding this comment.
Couldn't these be OBJECT libraries since the static libs themselves are never useful?
Owner
Author
There was a problem hiding this comment.
Yes, it could. But CMake's developers recommend to use STATIC libraries if there is no compelling reasons for OBJECT ones. For example, from Professional CMake: A Practical Guide 18th Edition
Prefer defining static libraries over object libraries. Static libraries are simpler, have more
complete and robust support from earlier CMake versions and they are well understood by most
developers. Object libraries have their uses, but they are also less flexible than static libraries.
hebasto
added a commit
that referenced
this pull request
May 1, 2024
5ca0799 fixup! cmake: Build `bitcoin_crypto` library (Hennadii Stepanov) Pull request description: Avoid using source-specific compile options. They hard to reason about and error-prone when combining with target-specific compile options. The resulted build logic effectively mirrors the master branch's one. Required for #157. Similar to #171. ACKs for top commit: TheCharlatan: ACK 5ca0799 Tree-SHA512: 8f55c86a3ad900c8de1789a97c936e39362d9995d75d2f9ca2d0f65f5863816795623d8ec1f2cc5f4468e27d934ce99150574cf766a7d6dd1da68546f13216fb
sedited
approved these changes
May 1, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Avoid using source-specific compile options. They are hard to reason about and error-prone when combining with target-specific compile options.
The resulted build logic effectively mirrors the master branch's one.
Required for #157.