Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes CMake setup across cores and examples by switching to OBJECT libraries, centralizing compile/link flags, and updating consumer CMake scripts. It also refactors the request API to use a nested kv field and tidies up example code formatting and casts.
- Switched all modular
add_library(...)toOBJECTlibraries and unified them into static/shared targets in the top-level CMakeLists. - Introduced
kvsub-struct inrequest_tand updated all reads to usereq->kv.key_size/val_size. - Updated example code (
printfformat specifiers,binary_fmt_strcasts, usage messages) and improved example CMakeLists to use pkg-config and proper find_package calls.
Reviewed Changes
Copilot reviewed 23 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/CMakeLists.txt | Link tests to unified ${PROJECT_NAME} and add includes |
| libCacheSim/utils/CMakeLists.txt | Switch utils_lib to OBJECT library |
| libCacheSim/traceReader/customizedReader/twrNSBin.h | Update req->val_size/key_size to nested kv |
| libCacheSim/traceReader/customizedReader/twrBin.h | Same kv refactor |
| libCacheSim/traceReader/customizedReader/oracle/*.h | Refactored kv accesses and fixed recursive return |
| libCacheSim/traceReader/CMakeLists.txt | Switch traceReader_lib to OBJECT library |
| libCacheSim/traceAnalyzer/reuse.h | Remove vtime_granularity_ field |
| libCacheSim/traceAnalyzer/CMakeLists.txt | Switch to OBJECT library, add include dirs |
| libCacheSim/profiler/CMakeLists.txt | Switch profiler_lib to OBJECT library |
| libCacheSim/mrcProfiler/CMakeLists.txt | Switch mrcProfiler_lib to OBJECT library |
| libCacheSim/include/libCacheSim/request.h | Introduce nested kv struct for key/val sizes |
| libCacheSim/dataStructure/CMakeLists.txt | Switch dataStructure_lib to OBJECT library |
| libCacheSim/cache/CMakeLists.txt | Switch cache_lib_c/cache_lib_cpp to OBJECT libs |
| example/cacheSimulatorConcurrent/main.cpp | Updated %lu to %lld casts |
| example/cacheSimulatorConcurrent/CMakeLists.txt | Modernized find_package, pkg-config usage |
| example/cacheSimulator/CMakeLists.txt | Modernized find_package, pkg-config usage |
| example/cacheHierarchy/simulator.cpp | Cast format string to (char*) |
| example/cacheHierarchy/main.cpp | Tidy up usage message (./ removed) |
| example/cacheHierarchy/CMakeLists.txt | Modernized find_package, pkg-config usage |
| example/cacheCluster/include/consistentHash.h | Make weight parameter const double * |
| example/cacheCluster/CMakeLists.txt | Modernized find_package, pkg-config usage |
| CMakeLists.txt (root) | Enable PIC, centralize dependencies into lists |
Comments suppressed due to low confidence (4)
example/cacheCluster/include/consistentHash.h:40
- Since the signature now takes
const double *weight, ensure the implementation inconsistentHash.cis updated to match this const qualifier.
ring_t *ch_ring_create_ring(int n_server, const double *weight);
test/CMakeLists.txt:26
- Before using
GLib_INCLUDE_DIRS, ensure you've calledfind_package(GLib REQUIRED)or imported the pkg-config module, since global includes were removed from the top-level CMake.
# Add include directories for test compilation
test/CMakeLists.txt:28
- [nitpick] Using
${CMAKE_SOURCE_DIR}may break if this CMakeLists is included from elsewhere; consider using${CMAKE_CURRENT_SOURCE_DIR}/../..or thelibCacheSim_include_dirvariable defined at the top-level.
${CMAKE_SOURCE_DIR}/libCacheSim # Add source dir to find utils/include/mymath.h
libCacheSim/traceAnalyzer/reuse.h:18
- The
vtime_granularity_paramandvtime_granularity_field were removed, but verify that no other methods referencevtime_granularity_; missing this may cause compilation errors or unintended behavior.
explicit ReuseDistribution(std::string output_path,
haochengxia
reviewed
Jun 25, 2025
Collaborator
|
LGTM! |
…rity - Set CMAKE_POSITION_INDEPENDENT_CODE to ON for better compatibility. - Transitioned from global include directories to target-specific includes for better encapsulation. - Changed library definitions from INTERFACE to OBJECT for modular libraries to avoid transitive dependencies. - Updated target linking to use the unified PROJECT_NAME for consistency across executables. - Enhanced installation targets for unified libraries and improved include directory management. - Cleaned up unnecessary global includes and streamlined dependency management across various components.
2ae89af to
8399b3c
Compare
8399b3c to
11710d1
Compare
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
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.
No description provided.