fix(scopetimer): harden sinks and require c++20 - #4
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3d65cb451
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #include <sys/stat.h> | ||
| #if defined(_WIN32) | ||
| #include <io.h> | ||
| #include <windows.h> |
There was a problem hiding this comment.
On Windows builds where the consumer has not already defined NOMINMAX, this public include causes windows.h to define min/max macros before the rest of ScopeTimer.hpp is parsed. The header later uses std::min(...) in inline functions, so those macros expand inside the header and break a default Windows CMake consumer, and they also leak to downstream files. Define NOMINMAX/WIN32_LEAN_AND_MEAN before including windows.h or avoid pulling it into the public header.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Retrospective disposition: accepted as an actionable review. This pull request merged without an explicit response, so I am not treating the merge as resolving the finding. The item is now carried as requiring verification against the repository's current main branch and, if still present, a focused follow-up fix. This thread will remain open until a correcting commit or concrete verification is linked.
| metric_name = "throughput_cost_per_record_us" | ||
| current_value = current_report.get(metric_name) | ||
| previous_value = previous_profile.get(metric_name) | ||
| if current_value is None and previous_value is None: |
There was a problem hiding this comment.
When the current report has the new throughput_cost_per_record_us key but the checked-in main baseline only has the legacy approx_per_record_us, current_value is non-null and previous_value is null, so this condition skips the legacy fallback and the comparison falls through to delta_mean_s. That makes BENCHMARK.md compare a different metric, or report unavailable, even though both per-record values exist under compatible names. Fall back per side before using mean overhead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Retrospective disposition: accepted as an actionable review. This pull request merged without an explicit response, so I am not treating the merge as resolving the finding. The item is now carried as requiring verification against the repository's current main branch and, if still present, a focused follow-up fix. This thread will remain open until a correcting commit or concrete verification is linked.
1 participant
Pull Request
Summary
Type Of Change
Motivation And Context
C++20 is now the supported baseline. The accompanying strict review fixes correctness, shutdown-safety, secure logging, and consumer-integration issues without removing intentional performance optimizations.
Testing
Validation: CTest in debug, release, strict AppleClang, strict GCC, ASan/UBSan, and TSan configurations; MinGW compilation; header coverage; ShellCheck, actionlint, pylint; and the MacBook benchmark matrix.
Contributor Checklist