Overhaul build, add sanitizer CI, implement process I/O#18
Conversation
376f246 to
83afb5f
Compare
83afb5f to
aca6f34
Compare
- Simplify source.inl and sink.inl to just include fd_source.hpp/fd_sink.hpp - Move all implementations to fd_source.inl and fd_sink.inl - Add static write_safe/read_safe helpers in io_native_handle_test.cpp to avoid sign-compare warnings when comparing with size_t Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove const/constexpr values from lambda capture lists since they don't need explicit capture (compiler handles them implicitly). Fixes -Wunused-lambda-capture warnings in Clang. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
string_view can be compared directly, no conversion needed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove MSVC version guard from fallback source_location::current() - MH_SOURCE_LOCATION_AUTO now always has a default value - Add missing string_insertion.hpp include in charconv test Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove StringMaker<std::string_view> from text_memstream_test.cpp - Remove StringMaker<std::byte> from data_bits_test.cpp - Catch2 v3 provides these definitions already - Add MH_COMPILE_LIBRARY_INLINE to thread_local static definition Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update Catch2 from 3.8.0 to 3.12.0 - Enable CATCH_CONFIG_CPP17_BYTE and CATCH_CONFIG_CPP17_STRING_VIEW - Set Catch2 to C++20 standard - Remove duplicate StringMaker definitions that conflict with Catch2 - Simplify source_location macros (always use ::current()) - Add 3 minute timeout to test runner - Remove clean recipe from justfile Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
CURL is required but wasn't being installed via apt. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
vcpkg was not being used - CURL is the only external dependency and can be installed directly via apt. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Increase epsilon in lerp_clamped test (1e-6 -> 1e-5) for floating point precision - Fix deadlock in process_manager: resume coroutines outside mutex lock to prevent pthread priority assertion failure Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix race condition in try_pop_task: hold lock for entire operation - Fix lost tasks: re-queue extra FD tasks instead of discarding them - Require unique_lock parameter to enforce locking at compile time - Add ASan, TSan, and UBSan CI jobs to catch threading/memory issues Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
TSan detected data races: - thread_pool::m_IsShuttingDown was accessed without synchronization - coroutine_task_test value variable was modified on thread pool and read on main thread without synchronization Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…erator Prevents undefined behavior when streaming a default-constructed source_location with null pointers. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The wait() function was returning as soon as the value/exception was set, but the worker thread might still be executing between set_state() and final_suspend(). This caused a data race when the main thread destroyed the task while the worker was still using the coroutine frame. Now wait() waits until both is_ready() AND final_suspend has run, ensuring the coroutine has fully completed before returning. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
auto-pr-resolve: failed — a human must take overWhat failed: model-call ceiling exceeded (1000 calls) The merge conflicts were NOT resolved. The trigger label was consumed when this run started — for another automated pass, re-apply the Full logs are on the webhook-runner dashboard. |
Conflicts resolved: - CMakeLists.txt: Two conflict blocks in CMakeLists.txt: the coroutine-support/fmt section and the mh_basic_install section. - cpp/include/mh/concurrency/dispatcher.inl: Three conflict blocks in try_pop_task() involving FD task re-queueing, delay task checking, and delay task time comparison. - cpp/include/mh/coroutine/task.hpp: Five conflict blocks in promise_base: wait(), wait_for() (x2), wait_until() (x2), and final_suspend(). - cpp/include/mh/io/fd_sink.inl: Four conflict blocks in fd_sink.inl: constructor init, write_async syscall, error throwing, and the post-#endif create_file/stdin_sink section. - cpp/include/mh/io/fd_source.inl: Four conflict blocks in fd_source.inl: constructor init, read_async syscall, error throwing, and the post-endif create_file/stdout/stderr section. - cpp/include/mh/math/interpolation.hpp: The constexpr round() helper's non-constant-evaluated fallback in interpolation.hpp. - cpp/include/mh/memory/unique_object.hpp: The default constructor's invalid-value initialization in unique_object.hpp. - cpp/include/mh/process/process.inl: Five conflict blocks in process.inl: includes, start() method body (posix_spawn vs fork+exec), I/O redirection setup, spawn call and cleanup, and the is_running/terminate/constructor section. - cpp/include/mh/process/process_manager.inl: Five conflict blocks in process_manager.inl: missing include, variable declaration in check_processes, waitpid result check, if/else body, and resume-outside-lock loop. - runtest.sh: runtest.sh was deleted in head but modified in base. - test/CMakeLists.txt: Three conflict blocks in test CMakeLists.txt about coverage, test registration, and getopt check. - test/containers_heap_test.cpp: containers_heap_test.cpp was added independently on both branches with entirely different test bodies. - test/data_bits_test.cpp: Include block and capture_value helper at the top of data_bits_test.cpp. - test/error_ensure_test.cpp: error_ensure_test.cpp was added independently on both branches with entirely different test bodies. - test/io_file_test.cpp: io_file_test.cpp was added independently on both branches with entirely different test bodies. - test/io_getopt_test.cpp: Include directives in io_getopt_test.cpp after the common headers. - test/memory_unique_object_test.cpp: memory_unique_object_test.cpp was added independently on both branches with entirely different test bodies. - test/text_charconv_helper_test.cpp: Conflicting include guards and test bodies at the top of text_charconv_helper_test.cpp. - test/text_memstream_test.cpp: Include block and to_str helper at the top of text_memstream_test.cpp. - vcpkg.json: vcpkg.json was deleted in head but modified in base.
There was a problem hiding this comment.
auto-pr-resolve: conflict in CMakeLists.txt
What conflicted: Two conflict blocks in CMakeLists.txt: the coroutine-support/fmt section and the mh_basic_install section.
Why it happened: Head removed the mh-cmake-common includes (mh-BasicInstall, mh-CheckCoroutineSupport) and the mh_check_cxx_coroutine_support call; base kept the coroutine check and added a fmt find_package/probe/link block plus CURL/fmt install dependencies for mh_basic_install.
Resolution: Block 1: removed the mh_check_cxx_coroutine_support call entirely (head removed the include(mh-CheckCoroutineSupport) that defines it, so keeping the call would be an undefined function) while keeping base's fmt support block that followed. Block 2: took base's mh_basic_install with FIND_DEPENDENCIES and the CURL/fmt install-deps setup, since base is the superset of head's bare mh_basic_install.
| if (!ready_fd_tasks.empty()) | ||
| { | ||
| if (ready_fd_tasks.size() > 1) | ||
| // Return first task, re-queue any extras (lock already held) |
There was a problem hiding this comment.
auto-pr-resolve: conflict in cpp/include/mh/concurrency/dispatcher.inl
What conflicted: Three conflict blocks in try_pop_task() involving FD task re-queueing, delay task checking, and delay task time comparison.
Why it happened: Head refactored to re-queue extra ready FD tasks inline (lock already held via unique_lock at top of function). Base added the same re-queue logic but wrapped it in a nested std::lock_guard on m_TasksMutex, which would deadlock since the lock is already held. Base also added notify_all() after re-queueing, which head omitted. For the delay task check, base re-added a redundant m_Tasks.empty() check and a nested lock_guard (deadlock), while head correctly just checks m_DelayTasks.empty().
Resolution: Took head's inline re-queue approach (no nested lock, correct since unique_lock already holds m_TasksMutex) but added base's m_TasksAvailableCV.notify_all() after re-queueing so waiting threads are woken. For the delay task check, took head's version (just m_DelayTasks.empty(), no nested lock, no redundant m_Tasks check since there is a separate m_Tasks block at line 126). For the time comparison, took head's version (captures now and front, compares m_DelayUntilTime).
There was a problem hiding this comment.
auto-pr-resolve: conflict in cpp/include/mh/coroutine/task.hpp
What conflicted: Five conflict blocks in promise_base: wait(), wait_for() (x2), wait_until() (x2), and final_suspend().
Why it happened: Head wanted wait() to also check m_FinalSuspendHasRun so callers wait until the coroutine fully reaches final_suspend, not just until the value is set. Head used is_ready() which takes the lock, but wait() already holds a unique_lock on m_Mutex, causing recursive locking. Base introduced is_ready_unlocked() to avoid the recursive lock but did not check m_FinalSuspendHasRun. For final_suspend, head used a suspend_sometimes that synchronously sets the flag and notifies; base introduced a final_suspend_awaiter that sets the flag in await_suspend (after the coroutine is fully suspended, making handle destruction safe).
Resolution: For wait/wait_for/wait_until: used base's is_ready_unlocked() (avoids recursive lock deadlock) combined with head's m_FinalSuspendHasRun check (plain bool read, safe in predicate). For final_suspend: took base's final_suspend_awaiter struct (safer destruction semantics) and added m_ValueReadyCV.notify_all() after setting m_FinalSuspendHasRun so waiters checking the flag are woken. Head's suspend_sometimes struct is no longer needed and was not included.
There was a problem hiding this comment.
auto-pr-resolve: conflict in cpp/include/mh/io/fd_sink.inl
What conflicted: Four conflict blocks in fd_sink.inl: constructor init, write_async syscall, error throwing, and the post-#endif create_file/stdin_sink section.
Why it happened: Head added is_open_(fd >= 0), a simple write call, runtime_error throws, and a duplicate create_file plus stdin_sink outside the ifdef. Base improved all of these: dup-failure-aware is_open_, EINTR retry, system_error throws, and moved create_file inside #ifdef unix. Base's create_file supersedes head's duplicate, but head's stdin_sink is new functionality not present in base.
Resolution: Blocks 1-3: took base's more robust versions (static_cast(fd_) for dup-failure awareness, EINTR retry loop, system_error with errno). Block 4: dropped head's duplicate create_file (base already defines it inside #ifdef unix with system_error) but kept head's new stdin_sink() function, then the closing brace.
There was a problem hiding this comment.
auto-pr-resolve: conflict in cpp/include/mh/io/fd_source.inl
What conflicted: Four conflict blocks in fd_source.inl: constructor init, read_async syscall, error throwing, and the post-endif create_file/stdout/stderr section.
Why it happened: Head added simple is_open, read call, runtime_error throws, and duplicate create_file plus stdout_source/stderr_source outside the ifdef. Base improved all with dup-failure-aware is_open, EINTR retry, system_error throws, and moved create_file inside the unix ifdef.
Resolution: Blocks 1-3 took base's robust versions. Block 4 dropped head's duplicate create_file (base already has it) but kept head's new stdout_source and stderr_source functions.
There was a problem hiding this comment.
auto-pr-resolve: conflict in cpp/include/mh/math/interpolation.hpp
What conflicted: The constexpr round() helper's non-constant-evaluated fallback in interpolation.hpp.
Why it happened: Head kept the original std::modf-based rounding; base rewrote it to handle NaN, large-magnitude values, and to avoid std::modf. Base's version is more robust.
Resolution: Took base's version: it handles NaN, large magnitudes, and uses intmax_t truncation instead of std::modf. This is a strict improvement over head's std::modf-based approach which had no NaN or overflow guards.
| #include <sys/wait.h> | ||
| #include <unistd.h> | ||
| #include <spawn.h> | ||
| #include <vector> |
There was a problem hiding this comment.
auto-pr-resolve: conflict in cpp/include/mh/process/process.inl
What conflicted: Five conflict blocks in process.inl: includes, start() method body (posix_spawn vs fork+exec), I/O redirection setup, spawn call and cleanup, and the is_running/terminate/constructor section.
Why it happened: Head implemented process I/O via posix_spawn with file_actions (the PR title says 'implement process I/O'). Base kept the old fork+exec approach that throws not_implemented_error for I/O redirection. Head's approach is the intended improvement and is a superset of base's functionality.
Resolution: Took head's posix_spawn approach for all start() conflicts since it implements I/O redirection (the PR's purpose). Added base's include to head's include set. For the final conflict, took head's is_running/terminate/constructor and dropped base's setup_child_io (not needed with posix_spawn).
There was a problem hiding this comment.
auto-pr-resolve: conflict in cpp/include/mh/process/process_manager.inl
What conflicted: Five conflict blocks in process_manager.inl: missing include, variable declaration in check_processes, waitpid result check, if/else body, and resume-outside-lock loop.
Why it happened: Both head and base independently refactored check_processes to resume coroutines outside the lock to avoid deadlock. Head used an if/else if/else chain (result greater than 0, result == -1, else) with a handles_to_resume vector. Base used a cleaner if(result==0) continue pattern with a to_resume vector. The non-conflicted lines (closing braces at 237-238, closing brace at 281, and resume loop at 283-288 using to_resume) constrain the structure to head's if/else chain approach. Base also added an explicit include of stdexcept for std::runtime_error.
Resolution: Took head's if/else if/else chain for the for loop body (matches the non-conflicted closing braces at lines 237-238). Renamed handles_to_resume to to_resume to match the non-conflicted resume loop at lines 283-288. Resolved the last conflict to an empty scope with a comment, pairing with the non-conflicted closing brace at line 281, so the actual resume loop at lines 283-288 is the only one. Added base's include of stdexcept.
There was a problem hiding this comment.
auto-pr-resolve: conflict in test/CMakeLists.txt
What conflicted: Three conflict blocks in test CMakeLists.txt about coverage, test registration, and getopt check.
Why it happened: Head uses a single unified test executable with file glob auto-discovery and ENABLE_COVERAGE gating. Base uses per-test mh_test executables with GCC-only coverage and an expanded test list. Head glob approach automatically picks up base new test files, so the unified executable is kept while preserving base special multi_tu_test and text_format_link_tu targets that need explicit CMake setup.
Resolution: Block 1 combined head ENABLE_COVERAGE gating with base GCC-only no-find_library approach. Block 2 kept head glob-based unified executable (auto-includes new tests) plus base multi_tu_test and text_format_link_tu targets. Block 3 kept head list REMOVE_ITEM approach with base quoted syntax.
|
|
||
| #include <functional> | ||
| #include <string> | ||
| #include <vector> |
There was a problem hiding this comment.
auto-pr-resolve: conflict in test/containers_heap_test.cpp
What conflicted: containers_heap_test.cpp was added independently on both branches with entirely different test bodies.
Why it happened: Head added a broad suite of heap functional tests (construction, push/pop, comparators, stress); base added focused regression tests for the initializer_list constructor's heap invariant. Both are valuable and non-overlapping.
Resolution: Combined both: merged the includes (head's , , last_include.hpp plus base's ) and kept both sets of test cases - head's broad functional tests followed by base's focused initializer_list-constructor regression tests.
There was a problem hiding this comment.
auto-pr-resolve: conflict in test/data_bits_test.cpp
What conflicted: Include block and capture_value helper at the top of data_bits_test.cpp.
Why it happened: Head added #include "last_include.hpp"; base expanded the includes with cstring, string, utility needed by the new bit_copy reference-implementation tests. Both sets of includes are needed.
Resolution: Combined both: kept base's expanded includes (cstddef, cstring, string, type_traits, utility) plus head's last_include.hpp, and the shared capture_value helper (identical on both sides).
There was a problem hiding this comment.
auto-pr-resolve: conflict in test/error_ensure_test.cpp
What conflicted: error_ensure_test.cpp was added independently on both branches with entirely different test bodies.
Why it happened: Head added a broad suite of ensure_traits/ensure_info/macro tests; base added focused regression tests for the standalone-include guard, debug-break declarations, and lvalue/rvalue forwarding behavior. Both are valuable and non-overlapping.
Resolution: Combined both: used base's standalone-include guard (undef MH_STUFF_API before including the header) and merged the includes (base's , , plus head's , , last_include.hpp), then kept both sets of test cases - head's broad ensure_traits/macro tests followed by base's focused debug-break/forwarding regression tests.
| @@ -1,10 +1,20 @@ | |||
| // The header must be usable standalone: it has to provide its own MH_STUFF_API | |||
There was a problem hiding this comment.
auto-pr-resolve: conflict in test/io_file_test.cpp
What conflicted: io_file_test.cpp was added independently on both branches with entirely different test bodies.
Why it happened: Head added a broad suite of read_file/write_file tests across char/wchar_t/C-string/large-file scenarios; base added focused regression tests for byte round-trip, empty files, the UTF-8 codecvt read-size bug, and missing-file throws. Both are valuable and non-overlapping.
Resolution: Combined both: used base's standalone-include guard (undef MH_STUFF_API before including the header) and merged the includes (head's catch_test_macros/catch_matchers_string, fstream, last_include.hpp plus base's locale, string), kept head's using namespace and base's global_locale_restorer/temp_file helpers, then kept both sets of test cases - head's broad read/write/round-trip/large-file tests followed by base's focused byte-round-trip/empty/UTF-8-regression/missing-file tests.
There was a problem hiding this comment.
auto-pr-resolve: conflict in test/io_getopt_test.cpp
What conflicted: Include directives in io_getopt_test.cpp after the common headers.
Why it happened: Head added #include "last_include.hpp" (the project's PCH-style final include); base added and needed by the new non-option-argument tests. Both are needed.
Resolution: Kept both: head's last_include.hpp plus base's and .
|
|
||
| #include <sstream> | ||
| #include <memory> | ||
| #include <utility> |
There was a problem hiding this comment.
auto-pr-resolve: conflict in test/memory_unique_object_test.cpp
What conflicted: memory_unique_object_test.cpp was added independently on both branches with entirely different test bodies.
Why it happened: Head added a broad suite of unique_object tests using IntTraits/PtrTraits/StatefulTraits (construction, move, release, reset, stream insertion, RAII); base added focused regression tests using a counting_handle_traits (default-constructed invalid value, double-delete, move transfer, move-only support). Both are valuable and non-overlapping.
Resolution: Combined both: merged the includes (head's sstream, memory, last_include.hpp plus base's utility), kept head's IntTraits/PtrTraits/StatefulTraits tests first, then base's counting_handle/move_probe/move_only tests in an anonymous namespace, preserving all test cases from both sides.
| @@ -1,8 +1,12 @@ | |||
| #include "mh/text/charconv_helper.hpp" | |||
There was a problem hiding this comment.
auto-pr-resolve: conflict in test/text_charconv_helper_test.cpp
What conflicted: Conflicting include guards and test bodies at the top of text_charconv_helper_test.cpp.
Why it happened: Head switched the Catch2 include from the vendored single-include header to <catch2/catch_all.hpp> and added a string_insertion test; base added comprehensive from_chars/to_chars tests using the new include style but kept the old guard #ifdef __cpp_lib_to_chars (vs head's #if __cpp_lib_to_chars >= 201611).
Resolution: Combined both: kept head's new <catch2/catch_all.hpp> include and string_insertion test, kept base's comprehensive from_chars/to_chars tests, and used base's stricter #if __cpp_lib_to_chars >= 201611 guard. The string_insertion test is unconditional (it doesn't need to_chars), so it stays outside the guard.
There was a problem hiding this comment.
auto-pr-resolve: conflict in test/text_memstream_test.cpp
What conflicted: Include block and to_str helper at the top of text_memstream_test.cpp.
Why it happened: Head added #include "last_include.hpp"; base expanded the includes with , , <type_traits> needed by the new stderr-capture and default-constructible tests. Both sets of includes are needed.
Resolution: Kept both: base's full set of includes (, , , <type_traits>) plus head's last_include.hpp, and the shared to_str helper (identical on both sides).
auto-pr-resolve: merged
|
| Path | Conflict | Resolution |
|---|---|---|
CMakeLists.txt |
both modified | merged content |
cpp/include/mh/concurrency/dispatcher.inl |
both modified | merged content |
cpp/include/mh/coroutine/task.hpp |
both modified | merged content |
cpp/include/mh/io/fd_sink.inl |
both modified | merged content |
cpp/include/mh/io/fd_source.inl |
both modified | merged content |
cpp/include/mh/math/interpolation.hpp |
both modified | merged content |
cpp/include/mh/memory/unique_object.hpp |
both modified | merged content |
cpp/include/mh/process/process.inl |
both modified | merged content |
cpp/include/mh/process/process_manager.inl |
both modified | merged content |
runtest.sh |
deleted in head, modified in base | deleted (took deletion) |
test/CMakeLists.txt |
both modified | merged content |
test/containers_heap_test.cpp |
both added | merged content |
test/data_bits_test.cpp |
both modified | merged content |
test/error_ensure_test.cpp |
both added | merged content |
test/io_file_test.cpp |
both added | merged content |
test/io_getopt_test.cpp |
both modified | merged content |
test/memory_unique_object_test.cpp |
both added | merged content |
test/text_charconv_helper_test.cpp |
both modified | merged content |
test/text_memstream_test.cpp |
both modified | merged content |
vcpkg.json |
deleted in head, modified in base | deleted (took deletion) |
19 of 20 model-resolved conflicts have inline review comments; the rest are explained below.
Resolution notes for files no longer in the PR's diff (an inline comment cannot anchor there):
cpp/include/mh/memory/unique_object.hpp — The default constructor's invalid-value initialization in unique_object.hpp.
Why it happened: Head changed the default init from m_Object{} to m_Object{Traits::invalid()}; base introduced a constexpr invalid_value() helper that calls Traits::invalid() when available and falls back to T{} otherwise. Base's approach is a strict superset of head's intent.
Resolution: Took base's version: the invalid_value() helper with if constexpr fallback supersedes head's direct Traits::invalid() call, since it handles both traits that define invalid() and those that don't (falling back to T{}).
Model: auto-pr-resolve (110 model call(s)).
CI still gates the merge — review the resolutions before merging.
Summary
This PR makes significant changes to the build system, CI, and I/O infrastructure:
justfilefor build/test tasks. Removed legacy Makefiles and batch scripts.fail-fasttotrue. Removed vcpkg setup step; now installslibcurl4-openssl-devdirectly.pipeclass for bidirectional communication. Addedsource::stdout_source(),source::stderr_source(),sink::stdin_sink()singletons. Addedconnect_io()utility. Addedsink::create_file()andsource::create_file()factory methods. Added singleton guards for standard stream file descriptors.fork/execvpwithposix_spawnpfor process creation. Implemented I/O redirection viaposix_spawn_file_actions. Parent now closes its copies of child's redirected FDs after spawn.task<T>::wait()to also wait form_FinalSuspendHasRunflag, preventing race conditions. Added notification on final suspend.try_pop_task()to hold lock across FD check and task queue operations. Renamedcheck_fd_tasks()tocheck_fd_tasks_locked().m_IsShuttingDowntostd::atomic<bool>.lerp_clamped/lerp_slow_clamped/remap/remap_clamped. Changed template parameters to accept different types for min/max. Addedcommon_type_tfor mixed-type operations.unique_objectdefault constructor to initialize withTraits::invalid()instead of default value.current(). Added null-pointer safety in stream insertion.mh_stuff_tests) with custom main that sets up dispatcher. Addedlast_include.hpppoison header. Addedcheck_last_include.shscript. Updated Catch2 to v3.12.0. Added many new tests (chrono, concurrency, containers, error, I/O, math, memory, source_location). Removed old Makefile-based test infrastructure.CLAUDE.mdfor AI coding assistant. Updated copyright year to 2025. Added.gitignoreentries for.claude/and.cache/. Removed empty.gitmodules. Added-Wno-missing-field-initializerscompiler flag. AddedMH_STUFF_STANDALONE_BUILDdefine.Testing
Risk
fork/exectoposix_spawn; behavior should be equivalent but may differ on some platforms.unique_objectdefault constructor change may affect code that relied on zero-initialization.