Skip to content

Refactor to C++23#6

Open
idotta wants to merge 11 commits intomasterfrom
refactor
Open

Refactor to C++23#6
idotta wants to merge 11 commits intomasterfrom
refactor

Conversation

@idotta
Copy link
Copy Markdown
Owner

@idotta idotta commented Mar 1, 2026

Note

High Risk
High risk because it rewrites core header parsing/serialization and sample conversion logic while also changing public headers/types (e.g., header field structs, error utilities), which can introduce subtle format/regression or downstream breaking changes.

Overview
This PR modernizes edfio to a C++23-first, CMake-built library with new packaging/CI: it adds a GitHub Actions CI matrix (Linux GCC/Clang, Windows MSVC, macOS), a root CMakeLists.txt for a header-only target with installable edfioConfig.cmake, and updates docs/ignore files.

It introduces a new high-level facade (EdfFile/EdfWriter) for opening, querying, reading samples/annotations, and writing records, while refactoring error handling by replacing Defs.hpp/Utils.hpp with Errors.hpp and adjusting call sites.

Core APIs and processors are substantially rewritten inline for C++23 (ranges/zip, std::format, std::from_chars, std::span/string_view), including data model updates (e.g., HeaderGeneral date/time structs, Record assignment/streaming changes) and a fix to ProcessorSampleRecord to correctly sign-extend multi-byte digital samples.

Written by Cursor Bugbot for commit 989b48f. This will update automatically on new commits. Configure here.

…header reading/writing

- Deleted ProcessorHeaderSignalFields, ProcessorSample, ProcessorSampleRecord, ProcessorTalRecord, ProcessorTimeStamp, and ProcessorTimeStampRecord implementations to clean up the codebase.
- Updated ReaderHeaderExam, ReaderHeaderGeneral, and ReaderHeaderSignal to directly implement reading logic without separate implementation files.
- Enhanced WriterHeaderExam, WriterHeaderGeneral, and WriterHeaderSignals to include error handling and direct writing logic.
- Improved overall structure and readability of header processing by consolidating related functionalities.
- Introduced Errors.hpp to define FileErrc enum for file-related errors with descriptive messages.
- Added ProcessorUtils.hpp containing utility functions for format checking, string manipulation, and parsing for various data types.
- Implemented functions to handle month conversion and string reduction, enhancing data processing capabilities.
… replacing std::copy with std::ranges::copy for improved readability and performance
…ability by modifying function signatures and consolidating loops
- Implemented tests for EDF+ format detection, annotation channels, and header fields in `test_edfplus.cpp`.
- Added tests for timestamp reading, TAL reading, and annotation processing in EDF+ files.
- Created tests for BDF+ format detection and annotation channels in `test_edfplus.cpp`.
- Introduced comprehensive tests for header signal processing and validation in `test_processors.cpp`.
- Included tests for annotation processing, sample conversion round-trip, and decimal string conversion in `test_processors.cpp`.
- Introduced EdfFile class to facilitate reading and writing EDF/BDF files.
- Implemented methods for accessing header information, reading physical and digital samples, and extracting annotations.
- Added EdfWriter class for writing EDF/BDF files, including automatic data record count patching on close.
- Created cmake configuration file for edfio.
- Updated ProcessorUtils to improve decimal string conversion.
- Added unit tests for EdfFile and EdfWriter functionalities, covering various scenarios including file reading, signal validation, and round-trip integrity checks.
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Free Tier Details

You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

double start{};
auto [ptr, ec] =
std::from_chars(value.data(), value.data() + value.size(), start);
if (ec != std::errc{} || ptr == value.data()) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from_chars rejects leading + in timestamp parsing

High Severity

ProcessTimeStampRecord calls std::from_chars directly on the record data, which starts with + or - (validated at line 30). The C++ standard specifies that std::from_chars for floating-point types does not recognize a leading + sign — only - is accepted. This means all positive EDF+ timestamps (the vast majority) will fail to parse, throwing an exception. The library's own detail::ParseDouble correctly strips the leading + before calling from_chars, but this function bypasses that helper.

Fix in Cursor Fix in Web

const auto &value = f();
os.write(value.data(), Sz);
return os;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field write overreads buffer after removing resize guard

Medium Severity

The refactored Field::operator<< removed the defensive value.resize(Sz, ' ') call that the old code performed before writing. Now os.write(value.data(), Sz) reads Sz bytes regardless of the actual string length. If a Field is never populated via its setter, its internal string is empty, causing a buffer overread. This happens for m_reserved (Field<44>) when writing plain EDF/BDF files — ProcessHeaderGeneral conditionally sets it only when m_reserved is non-empty or the format is Plus.

Additional Locations (1)

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant