Note on provenance: This issue was found by Claude Opus 5 while reviewing #325. The reproductions and output shown below were produced by building and running the code, but I have only briefly reviewed the analysis myself and I am not an expert in this area. Please review it critically.
Impact
Writing your own BaseIO backend is an advertised feature of AqNWB. The caching refactor changed getDataSet from a public pure virtual into a plain non virtual method, and introduced a new protected pure virtual getDataSetImpl in its place. Any out of tree backend that implemented the old method now fails to compile, with two errors and no pointer toward what to do instead.
The CHANGELOG does not mention it. getDataSet does not appear in that file at all, so someone reading the release notes to plan an upgrade gets no warning.
What happens
A backend that compiled against the previous revision:
class MyIO : public AQNWB::IO::BaseIO {
std::shared_ptr<BaseRecordingData> getDataSet(const std::string& path) override;
// ... every other pure virtual implemented
};
now fails:
error: only virtual member functions can be marked 'override'
std::shared_ptr<BaseRecordingData> getDataSet(const std::string& path) override
^~~~~~~~
error: variable type 'MyIO' is an abstract class
MyIO io;
^
note: unimplemented pure virtual method 'getDataSetImpl' in 'MyIO'
The fix for an integrator is small once you know it: rename the override to getDataSetImpl and move it to the protected section. The problem is discoverability.
What changed
Before, public:
virtual std::shared_ptr<BaseRecordingData> getDataSet(const std::string& path) = 0;
After, src/io/BaseIO.hpp:915, public and non virtual:
std::shared_ptr<BaseRecordingData> getDataSet(const std::string& path, bool reset = false);
with src/io/BaseIO.hpp:1018, protected:
virtual std::shared_ptr<BaseRecordingData> getDataSetImpl(...) = 0;
The in tree backend was migrated the same way, HDF5IO.hpp:458.
Also missing from the CHANGELOG
RecordingObjects::clearRecordingDataCache() was a public method and is now deleted outright. The equivalent behaviour lives on BaseIO, but the existing **[BREAKING]** entry describes only the RegisteredType side of the move, and RegisteredType::clearRecordingDataCache still exists as a deprecated forwarder. So the entry reads as a soft deprecation while an actual removal went unlisted.
Suggested direction
Add a **[BREAKING]** CHANGELOG entry covering both, with a short migration note along the lines of "custom BaseIO backends should rename getDataSet to getDataSetImpl and move it to protected".
Small related nit: the class diagram in docs/pages/devdocs/record_design.dox:65 still lists + getDataSet(): BaseRecordingData on the HDF5IO node, which no longer declares it. That diagram was edited in this PR to add m_recordingDataCache, so the stale row is easy to update at the same time.
Impact
Writing your own
BaseIObackend is an advertised feature of AqNWB. The caching refactor changedgetDataSetfrom a public pure virtual into a plain non virtual method, and introduced a new protected pure virtualgetDataSetImplin its place. Any out of tree backend that implemented the old method now fails to compile, with two errors and no pointer toward what to do instead.The CHANGELOG does not mention it.
getDataSetdoes not appear in that file at all, so someone reading the release notes to plan an upgrade gets no warning.What happens
A backend that compiled against the previous revision:
now fails:
The fix for an integrator is small once you know it: rename the override to
getDataSetImpland move it to the protected section. The problem is discoverability.What changed
Before, public:
After,
src/io/BaseIO.hpp:915, public and non virtual:with
src/io/BaseIO.hpp:1018, protected:The in tree backend was migrated the same way,
HDF5IO.hpp:458.Also missing from the CHANGELOG
RecordingObjects::clearRecordingDataCache()was a public method and is now deleted outright. The equivalent behaviour lives onBaseIO, but the existing**[BREAKING]**entry describes only theRegisteredTypeside of the move, andRegisteredType::clearRecordingDataCachestill exists as a deprecated forwarder. So the entry reads as a soft deprecation while an actual removal went unlisted.Suggested direction
Add a
**[BREAKING]**CHANGELOG entry covering both, with a short migration note along the lines of "customBaseIObackends should renamegetDataSettogetDataSetImpland move it toprotected".Small related nit: the class diagram in
docs/pages/devdocs/record_design.dox:65still lists+ getDataSet(): BaseRecordingDataon theHDF5IOnode, which no longer declares it. That diagram was edited in this PR to addm_recordingDataCache, so the stale row is easy to update at the same time.