Introduce more flexible storeChunk() syntax, use to add ADIOS2 memory selection#6
Open
franzpoeschel wants to merge 62 commits intodevfrom
Open
Introduce more flexible storeChunk() syntax, use to add ADIOS2 memory selection#6franzpoeschel wants to merge 62 commits intodevfrom
franzpoeschel wants to merge 62 commits intodevfrom
Conversation
* [pre-commit.ci] pre-commit autoupdate updates: - [github.com/pre-commit/mirrors-clang-format: v21.1.8 → v22.1.0](pre-commit/mirrors-clang-format@v21.1.8...v22.1.0) - [github.com/pycqa/isort: 7.0.0 → 8.0.1](PyCQA/isort@7.0.0...8.0.1) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Fix typing issues with load/store_chunk in Python * Datatype helpers: non-template variants * Unify Datatype equality semantics * replace operator==(Datatype, Datatype) by isSame
…D#1860) * attempt to fix issue openPMD#1859 by caching the index and avoid indexOf() call for iterations --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Franz Pöschel <franz.poeschel@gmail.com>
* Do not flush if the backend does not support Span API * Fix flushing logic
…PMD#1851) * Fix keepalives * Add simple GC test * Add keepalive for snapshots api * Add more extensive keepalive test * Slightly API-breaking.. need to del everything * tmp, check sth * tmp check ci * Revert "tmp check ci" This reverts commit 1cf6973. * Revert "tmp, check sth" This reverts commit 93ed467. * Fix typing issues with load/store_chunk in Python
These are prefixed by test*, so they already run separately.
Replicate a Golang-inspired defer pattern in order to ensure resource cleanup also upon early return. --------- Co-authored-by: AI Agent <ai@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (48)
📝 WalkthroughWalkthroughAdds a fluent ConfigureLoadStore API for chunk load/store with deferred execution, introduces MemorySelection support, refactors flush into a non-virtual wrapper + protected hook with flush-counter tracking, enhances datatype comparison helpers (isSame/isSigned), adds deferred-computation and defer RAII utilities, and updates multiple backends, bindings, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant RC as RecordComponent
participant CLS as ConfigureLoadStore
participant IOH as IOHandler
participant Backend as Storage Backend
RC->>CLS: prepareLoadStore() -> ConfigureLoadStore
CLS->>CLS: configure offset/extent/memorySelection
CLS->>CLS: bind buffer (shared/unique/raw/contiguous)
CLS->>CLS: enqueue storeSpan / load -> DeferredComputation
Note over CLS,IOH: DeferredComputation executes later
CLS->>IOH: enqueue Operation (WRITE/READ) with params (offset,extent,memorySelection)
IOH->>Backend: perform backend operations (apply memory selection if supported)
Backend-->>IOH: completion
IOH-->>CLS: operation result
CLS-->>RC: return result / shared_ptr buffer
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/auxiliary/Filesystem.cpp (3)
119-169:⚠️ Potential issue | 🟠 MajorPreserve parent bits for relative paths too.
For
create_directories("foo"), the firstmk()receivesfoo/;get_parent("foo/")becomesfoo, thenget_permissions("foo")resolves to an empty parent and returns0. That skips sticky/setgid preservation from the current directory.🐛 Proposed fix
- std::string get_parent(std::string const &path) + std::string get_parent(std::string path) { - std::string parent = path; - size_t pos = parent.find_last_of(directory_separator); - if (pos != std::string::npos) + while (path.size() > 1 && path.back() == directory_separator) + { + path.pop_back(); + } + + size_t pos = path.find_last_of(directory_separator); + if (pos == std::string::npos) { - parent = parent.substr(0, pos); - if (parent.empty()) - parent = "/"; + return "."; } - else + if (pos == 0) { - parent.clear(); + return std::string(1, directory_separator); } - return parent; + return path.substr(0, pos); } - mode_t get_permissions(std::string const &path) + mode_t get_permissions(std::string const &directory) { - std::string parent = get_parent(path); - if (parent.empty() || !directory_exists(parent)) - { - return 0; - } - struct stat s; - if (stat(parent.c_str(), &s) != 0) + if (stat(directory.c_str(), &s) != 0 || !S_ISDIR(s.st_mode)) { return 0; } return s.st_mode & 07777; @@ // preserve sticky and setgid from parent mode_t parentPerms = - get_permissions(get_parent(p)) & (S_ISVTX | S_ISGID); + get_permissions(get_parent(p)) & (S_ISVTX | S_ISGID); return (0 == mkdir(p.c_str(), 0777 | parentPerms));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auxiliary/Filesystem.cpp` around lines 119 - 169, get_permissions currently returns 0 when get_parent(path) yields an empty string, which causes create_directories/mk to lose sticky/setgid bits for relative single-component paths like "foo"; change get_permissions (or its caller) so that if parent is empty it uses the current directory (e.g. treat parent as "." ) before checking directory_exists or calling stat, so get_permissions can return the actual parent mode bits; update references in get_permissions/get_parent/create_directories so mk() preserves S_ISVTX|S_ISGID from "." for relative paths.
107-110:⚠️ Potential issue | 🟠 MajorDetect
readdirfailures before returning a partial listing.
readdir()returnsnullptrboth at end-of-directory and on error. Without clearing and checkingerrno,list_directory_nothrow()can silently return a partial vector as success instead ofstd::nullopt.Proposed fix
dirent *entry; + errno = 0; while ((entry = readdir(directory)) != nullptr) if (strcmp(entry->d_name, ".") != 0 && strcmp(entry->d_name, "..") != 0) ret.emplace_back(entry->d_name); - closedir(directory); + if (errno != 0) + { + int const savedErrno = errno; + closedir(directory); + errno = savedErrno; + return std::nullopt; + } + if (closedir(directory) != 0) + { + return std::nullopt; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auxiliary/Filesystem.cpp` around lines 107 - 110, The function list_directory_nothrow currently iterates with readdir and can return a partial listing because readdir returns nullptr on both end-of-dir and error; fix it by clearing errno before the loop (errno = 0), using while ((entry = readdir(directory)) != nullptr) to collect names into ret, then after the loop check if errno != 0 to detect an error; on error, ensure you close the DIR* (closedir(directory)), clear/ignore ret and return std::nullopt, otherwise close directory and return ret as before—apply this change around the existing readdir loop and closedir handling in list_directory_nothrow.
90-99:⚠️ Potential issue | 🟠 MajorHonor the
nothrowcontract on Windows.The
_WIN32path violates the nothrow contract by throwing instead of returningstd::nulloptonFindFirstFilefailure (line 91). Additionally, the code useserrnowith Win32 APIs, which is incorrect—Win32FindFirstFile/FindNextFilereport errors viaGetLastError, noterrno. TheFindNextFileloop also lacks a post-loop error check to distinguish normal end-of-directory (ERROR_NO_MORE_FILES) from actual failures. This causesremove_directory()to unexpectedly throw on Windows.Proposed fix
HANDLE hFind = FindFirstFile(pattern.c_str(), &data); if (hFind == INVALID_HANDLE_VALUE) - throw std::system_error(std::error_code(errno, std::system_category())); + return std::nullopt; do { if (strcmp(data.cFileName, ".") != 0 && strcmp(data.cFileName, "..") != 0) ret.emplace_back(data.cFileName); } while (FindNextFile(hFind, &data) != 0); + auto const lastError = GetLastError(); FindClose(hFind); + if (lastError != ERROR_NO_MORE_FILES) + return std::nullopt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auxiliary/Filesystem.cpp` around lines 90 - 99, The Windows branch violates the nothrow contract by throwing on FindFirstFile failure and misuses errno; update the logic in the routine that enumerates directory entries (the block using FindFirstFile/FindNextFile) to return std::nullopt instead of throwing when FindFirstFile returns INVALID_HANDLE_VALUE, use GetLastError() to capture Win32 error codes (not errno), and after the FindNextFile loop check GetLastError() to distinguish ERROR_NO_MORE_FILES from real failures; ensure FindClose(hFind) is always called on a valid handle before returning and surface a std::nullopt on any real Win32 error.include/openPMD/IO/DummyIOHandler.hpp (1)
43-48:⚠️ Potential issue | 🟠 MajorMake
flush_implnon-public in all IOHandler implementations.With the
flush()wrapper +flush_impl()hook split,flush_implis intended as an internal customization point (as documented inAbstractIOHandlerwhere it's correctly declaredprotected). However,flush_implis currently declaredpublicinDummyIOHandler,HDF5IOHandler,JSONIOHandler, andParallelHDF5IOHandler, allowing external callers to bypass the wrapper's pre/post logic. Change all overrides toprotectedto maintain the Non-Virtual Interface pattern and prevent direct bypass of the publicflush()wrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/openPMD/IO/DummyIOHandler.hpp` around lines 43 - 48, The override of the internal hook flush_impl should not be public: change the declaration of flush_impl(internal::ParsedFlushParams &) override from public to protected in DummyIOHandler (and do the same in HDF5IOHandler, JSONIOHandler, ParallelHDF5IOHandler) so callers cannot bypass the public flush() wrapper; keep the signature and the override specifier unchanged to preserve the customization point declared protected in AbstractIOHandler.src/IO/ADIOS/ADIOS2IOHandler.cpp (1)
1300-1325:⚠️ Potential issue | 🟠 MajorKeep
queryOnlyconsistent with the real span eligibility checks.Line 1323 reports buffer-view support for every opt-in engine before honoring
UseSpan::Noand the BP5/version auto-disable branch below. A frontend query can therefore choose the backend-managed path even though the subsequent realgetBufferViewcall returnsbackendManagedBuffer = false.🛠️ Proposed fix direction
else if (parameters.queryOnly) { - parameters.out->backendManagedBuffer = true; + parameters.out->backendManagedBuffer = + m_useSpanBasedPutByDefault != UseSpan::No +#if ( \ + ADIOS2_VERSION_MAJOR * 1000 + ADIOS2_VERSION_MINOR * 10 + \ + ADIOS2_VERSION_PATCH) <= 2102 + && this->realEngineType() != "bp5" +#endif + ; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IO/ADIOS/ADIOS2IOHandler.cpp` around lines 1300 - 1325, The current getBufferView implementation sets parameters.out->backendManagedBuffer = true when parameters.queryOnly is true even for engines that will later be rejected by the real span-eligibility checks (optInEngines, BP5/version auto-disable, UseSpan::No), causing inconsistent responses; update the logic in ADIOS2IOHandlerImpl::getBufferView so that parameters.queryOnly only causes backendManagedBuffer = true after the same full eligibility checks used for the real getBufferView call succeed (i.e., verify realEngineType() is in optInEngines and run the BP5/version and UseSpan::No checks before honoring parameters.queryOnly), or alternatively compute a single boolean like canUseSpan (based on optInEngines, BP5/version handling, and UseSpan::No) and use that both for the queryOnly branch and the real call so backendManagedBuffer is consistent.include/openPMD/backend/Attributable.hpp (1)
30-36:⚠️ Potential issue | 🟠 MajorInclude
<cstdint>before usinguint8_t.Line 593 uses
uint8_tas the underlying type forEnqueueAsynchronously, but this header does not directly include<cstdint>. Relying on transitive includes creates portability risk and can cause compilation failures on stricter toolchains or with different include orders.🛠️ Proposed fix
`#include` <cstddef> +#include <cstdint> `#include` <map>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/openPMD/backend/Attributable.hpp` around lines 30 - 36, The header uses uint8_t (as the underlying type for EnqueueAsynchronously) but doesn't include <cstdint>, which risks relying on transitive includes; add `#include` <cstdint> to include/openPMD/backend/Attributable.hpp (alongside the existing includes) so uint8_t is defined before the declaration/definition of EnqueueAsynchronously and any code referencing it (search for the EnqueueAsynchronously symbol to verify placement).src/binding/python/Series.cpp (2)
502-508:⚠️ Potential issue | 🟡 MinorStale comment after
keep_alivedirection reversal.The comment "garbage collection: return value must be freed before Series" describes the previous
py::keep_alive<1, 0>()semantics (patient0kept alive by nurse1). Withpy::keep_alive<0, 1>(), it is the other way around: the Series (patient1) is kept alive for as long as the returnediterations(nurse0) lives. Please update the comment to avoid misleading future readers.📝 Proposed comment update
- py::return_value_policy::copy, - // garbage collection: return value must be freed before Series - py::keep_alive<0, 1>())) + py::return_value_policy::copy, + // keep Series alive for as long as the returned `iterations` + // view is in use + py::keep_alive<0, 1>()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/binding/python/Series.cpp` around lines 502 - 508, The comment on the py::cpp_function exposing Series::iterations is stale for py::keep_alive<0, 1>(); update it to accurately state the semantics of py::keep_alive<0, 1>() (i.e., the Series object (patient 1) is kept alive for as long as the returned iterations object (nurse 0) lives) and remove or replace the existing "garbage collection: return value must be freed before Series" wording; reference the Series class, the iterations property, and py::keep_alive<0, 1>() when editing.
618-663:⚠️ Potential issue | 🔴 CriticalCritical:
__del__is bound on the module, not theSeriesclass.The
.def("__del__", ...)at line 650 is chained ontom.def("merge_json", ...)(line 618), andpy::module_::def(...)returnsmodule_&. This binds a module-level free function rather than installing a Python destructor on theSeriesclass. As a result, Python garbage collection ofSeriesinstances will never invoke this cleanup handler.🛠️ Proposed fix: attach `__del__` to the class binding `cl`
m.def( "merge_json", py::overload_cast<std::string const &, std::string const &>( &json::merge), py::arg("default_value") = "{}", py::arg("overwrite") = "{}", docs_merge_json) `#if` openPMD_HAVE_MPI .def( "merge_json", [](std::string const &default_value, std::string const &overwrite, py::object &comm) { /* ... */ }, py::arg("default_value") = "{}", py::arg("overwrite") = "{}", py::arg("comm"), docs_merge_json) -#endif - .def("__del__", [](Series &s) { - try - { - s.close(); - } - catch (std::exception const &e) - { - std::cerr << "Error during close: " << e.what() << std::endl; - } - catch (...) - { - std::cerr << "Unknown error during close." << std::endl; - } - }); +#endif + ; + + cl.def("__del__", [](Series &s) { + try + { + s.close(); + } + catch (std::exception const &e) + { + std::cerr << "Error during close: " << e.what() << std::endl; + } + catch (...) + { + std::cerr << "Unknown error during close." << std::endl; + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/binding/python/Series.cpp` around lines 618 - 663, The __del__ destructor is currently being chained onto the module binding (m.def(...)) so it's bound at module level instead of to the Series class; move the .def("__del__", ...) call so it is invoked on the class binding object (cl) that defines Series (i.e., attach the lambda that calls s.close() and swallows exceptions to cl via cl.def("__del__", ...)), ensuring the destructor is registered on the Series class rather than on the module.src/IO/HDF5/HDF5IOHandler.cpp (2)
2565-2615:⚠️ Potential issue | 🔴 CriticalPremature closure of member datatype handles corrupts class state.
The defer guard captures
dataTypeby reference. When lines 2595 and 2613 reassign it tom_H5T_LONG_DOUBLE_80_LEandm_H5T_CLONG_DOUBLE_80_LErespectively, the guard's reference now points to class member handles. The guard then closes these member handles at function exit, while the destructor attempts to close them again, causing double-close errors and leaving the handler with invalid datatype IDs.Save the original handle before reassignment and close only that:
Fix
hid_t dataType = getH5DataType(a); + hid_t dataTypeToClose = dataType; auto defer_close_dataType = auxiliary::defer([&]() { - status = H5Tclose(dataType); + status = H5Tclose(dataTypeToClose); if (status != 0) { std::cerr << "[HDF5] Internal error: Failed to close dataset datatype "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IO/HDF5/HDF5IOHandler.cpp` around lines 2565 - 2615, The defer guard currently captures dataType by reference so when dataType is reassigned to member handles (m_H5T_LONG_DOUBLE_80_LE / m_H5T_CLONG_DOUBLE_80_LE) the guard will close those members later; to fix, capture and close the original dataset type handle only by storing it in a local variable (e.g. origDataType = dataType) before any potential reassignment and update the defer_close_dataType to close origDataType (or capture by value), leaving dataType free to be reassigned after the check with checkDatasetTypeAgain (use H5Dget_type and H5Tequal as already present) so member handles are not closed by the local defer.
1528-1658:⚠️ Potential issue | 🔴 CriticalDo not close
next_typebefore reusing it asdataset_type.At line 1641, a defer guard is installed that closes
next_typeat the end of the else block scope. However, line 1657 assigns this handle todataset_typefor the next loop iteration, causing the HDF5 APIs on line 1579+ to operate on a closed handle. Additionally, the originaldataset_typehandle is never closed before reassignment, causing a resource leak.🐛 Proposed handle-lifetime fix
- auto defer_close_next_type = auxiliary::defer([&]() { - status = H5Tclose(next_type); - if (status != 0) - { - std::cerr - << "[HDF5] Internal error: Failed to close HDF5 " - "dataset type during dataset opening." - << std::endl; - } - }); - if (H5Tequal(dataset_type, next_type)) { + status = H5Tclose(next_type); + if (status != 0) + { + std::cerr + << "[HDF5] Internal error: Failed to close HDF5 " + "dataset super type during dataset opening." + << std::endl; + } throw_error(); } + hid_t previous_type = dataset_type; dataset_type = next_type; + status = H5Tclose(previous_type); + VERIFY( + status == 0, + "[HDF5] Internal error: Failed to close HDF5 dataset type " + "during dataset opening."); --remaining_tries; repeat = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IO/HDF5/HDF5IOHandler.cpp` around lines 1528 - 1658, The loop incorrectly installs a defer that closes next_type then assigns next_type to dataset_type (causing use-after-close) and also never closes the old dataset_type (leak). Fix by removing the defer that closes next_type inside the else branch; instead, before reassigning, call H5Tclose(dataset_type) to close the current dataset_type handle, then set dataset_type = next_type and ensure the existing defer_close_dataset_type (or a single auxiliary::defer managing dataset_type) will close the new dataset_type at scope exit; keep references to H5Tget_super, H5Tclose, H5Tequal, dataset_type, next_type, and defer_close_dataset_type to locate and adjust the logic.
🧹 Nitpick comments (9)
include/openPMD/Dataset.hpp (1)
37-47: Optional: document/validate dimensional consistency betweenoffsetandextent.As declared, nothing prevents
offset.size() != extent.size()or rank mismatches against the dataset/chunk being stored; users will only discover errors deep inside ADIOS2 or other backends. Consider either (a) a short docstring note stating the invariant (offset.size() == extent.size()and must match the memory buffer's dimensionality) or (b) a small helper constructor /valid()check to catch obvious misuse early. Not a blocker since the type is trivial and backend-side validation is presumably present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/openPMD/Dataset.hpp` around lines 37 - 47, Add a short invariant note and a lightweight validation helper for MemorySelection: document that MemorySelection::offset.size() must equal MemorySelection::extent.size() and that both must match the dimensionality of the target dataset/chunk, and add a small member function bool valid() const (or a constructor overload) on struct MemorySelection that returns false if offset.size() != extent.size() (and optionally checks for non-negative extents/offsets); update the struct comment to state the invariant and reference MemorySelection::valid() so callers can perform an early check before passing to backends like ADIOS2.include/openPMD/backend/PatchRecordComponent.hpp (1)
121-214: LGTM —isSame(...)replaces directDatatypeequality.Matches the new
isSamesemantics (aliased-integer-equivalent types such aslongvslong longon platforms where they have identical representation) and fixes the long-standing Windowsoperator==caveat called out in the comment on Line 125. The three sites (load,store(idx, T),store(T)) are updated consistently.Minor note: the error messages in
store(...)say "Datatypes … do not match", which is slightly misleading now sinceisSameaccepts representationally-equivalent-but-distinct enum values. Consider rewording to e.g. "are not representation-compatible" if you want the message to reflect the new semantics; not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/openPMD/backend/PatchRecordComponent.hpp` around lines 121 - 214, Update the error messages in PatchRecordComponent::store(uint64_t idx, T) and PatchRecordComponent::store(T) to reflect the new isSame semantics: replace "Datatypes of patch data (...) and dataset (...) do not match." with wording that indicates representational compatibility (e.g. "Datatypes of patch data (...) and dataset (...) are not representation-compatible.") so the message matches that isSame allows aliased-but-equivalent enum values; keep the existing dtype/getDatatype() details in the message.test/SerialIOTest.cpp (1)
1760-1770: Assert the loaded variant data, not only print it.This test would still pass if
prepareLoadStore().loadVariant().get()returned the wrong value. Since the dataset is known to bedoubleand the first value is0.0, please add a direct assertion for both load paths.🧪 Proposed test strengthening
auto variantTypeDataset = rc.loadChunkVariant(); auto variantTypeDataset2 = rc.prepareLoadStore().loadVariant().get(); rc.seriesFlush(); + REQUIRE(std::holds_alternative<std::shared_ptr<double>>( + variantTypeDataset)); + REQUIRE(std::holds_alternative<std::shared_ptr<double>>( + variantTypeDataset2)); + REQUIRE(std::get<std::shared_ptr<double>>(variantTypeDataset).get()[0] == + 0.0); + REQUIRE(std::get<std::shared_ptr<double>>(variantTypeDataset2).get()[0] == + 0.0); for (auto ptr : {&variantTypeDataset, &variantTypeDataset2}) { std::visit(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/SerialIOTest.cpp` around lines 1760 - 1770, The test currently only prints the first loaded value for variantTypeDataset and variantTypeDataset2; add assertions that verify the loaded variant's first element equals 0.0 for both load paths instead of just printing. Locate the loop over {&variantTypeDataset, &variantTypeDataset2} and inside the std::visit lambda (used with prepareLoadStore().loadVariant().get() results after rc.seriesFlush()) replace or augment the std::cout with an assertion (e.g., ASSERT_DOUBLE_EQ/EXPECT_DOUBLE_EQ or appropriate test macro) that checks shared_ptr.get()[0] == 0.0 for every visited variant.include/openPMD/IO/ADIOS/macros.hpp (1)
52-69: Add explicit standard headers forstd::declvaland make trait return-type agnostic.
std::declvalis used without direct<utility>include, relying on transitive inclusion through<adios2.h>. The specialization only matches whenSetMemorySelection()returnsvoid(the default SFINAE parameter). Usingstd::void_tand explicit standard headers improves robustness and portability.Proposed improvement
`#include` "openPMD/config.hpp" `#if` openPMD_HAVE_ADIOS2 `#include` <adios2.h> + +#include <type_traits> +#include <utility> `#define` openPMD_HAS_ADIOS_2_10 \ (ADIOS2_VERSION_MAJOR * 100 + ADIOS2_VERSION_MINOR >= 210) @@ - template <typename Variable, typename SFINAE = void> - struct CanTheMemorySelectionBeReset - { - static constexpr bool value = false; - }; + template <typename Variable, typename SFINAE = void> + struct CanTheMemorySelectionBeReset : std::false_type + {}; template <typename Variable> struct CanTheMemorySelectionBeReset< Variable, - decltype(std::declval<Variable>().SetMemorySelection())> - { - static constexpr bool value = true; - }; + std::void_t< + decltype(std::declval<Variable &>().SetMemorySelection())>> + : std::true_type + {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/openPMD/IO/ADIOS/macros.hpp` around lines 52 - 69, Include the proper standard headers (<utility> and <type_traits>), change the SFINAE specialization in detail::CanTheMemorySelectionBeReset to use std::void_t to detect the presence of a SetMemorySelection member regardless of its return type (e.g. use template<typename V, typename = std::void_t<decltype(std::declval<V>().SetMemorySelection())>>), and make the top-level CanTheMemorySelectionBeReset a templated constexpr variable template (instead of hardcoding adios2::Variable<int>) so callers can query CanTheMemorySelectionBeReset<SomeVarType>.include/openPMD/IO/AbstractIOHandler.hpp (1)
267-283: Consider tightening visibility/placement ofm_flushCounter.
m_flushCounteris declared in thepublic:section, interleaved between the twoflush(...)overload declarations. Since the field is described as an internal coordination mechanism betweenAbstractIOHandler::flush()and deferred-computation consumers ofloadStoreChunk(), it would be cleaner to move it down next to the other non-public bookkeeping members (e.g. nearm_lastFlushSuccessful/m_work) and, if possible, give itprotectedvisibility. As currently written, any external caller can reset or reassign the shared pointer and desynchronize the counter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/openPMD/IO/AbstractIOHandler.hpp` around lines 267 - 283, Move the public m_flushCounter member into the non-public bookkeeping area of AbstractIOHandler (near m_lastFlushSuccessful and m_work) and change its visibility to protected so subclasses can access it but external callers cannot reset/reassign it; ensure the existing initialization (std::make_shared<unsigned long long>(0)) stays intact and that the two overloads flush(internal::FlushParams) and flush(internal::ParsedFlushParams&) still reference the member correctly after the move.test/ParallelIOTest.cpp (1)
538-562: Minor:last_rowis loaded unconditionally but asserted conditionally.When
CanTheMemorySelectionBeResetisfalse, the last row is never written, yet Line 538 still issues aload<int>()for offset{4, 0}. The contents are then not asserted, so this is harmless, but it silently pulls unwritten data and could confuse future readers/debuggers. Consider guarding the load itself behind the sameif constexprso the read request only happens when the data was written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ParallelIOTest.cpp` around lines 538 - 562, The test unconditionally issues a load for last_row via E_y.prepareLoadStore().offset({4, 0}).load<int>().get() even when CanTheMemorySelectionBeReset is false and that row was never written; guard the load behind the same constexpr condition to avoid pulling unwritten data. Modify the block so that last_row is only constructed/loaded when CanTheMemorySelectionBeReset is true (matching the later assertion loop), keeping the existing checks that iterate over {&first_row, &last_row} when true and only &first_row when false. Ensure references to last_row in the subsequent loops are only present when the constexpr branch includes it (use the same lambda/return pattern already used).test/Files_Core/read_nonexistent_attribute.cpp (1)
23-63: Minor: missing<algorithm>include, and unused Catch include in stub branch.
std::remove_ifis used at Line 51 but only<catch2/catch_tostring.hpp>and<catch2/catch_test_macros.hpp>are included directly; this compiles today only by accident via transitive includes fromCoreTests.hpp. Please add an explicit#include <algorithm>(and#include <vector>,#include <string>) inside the invasive branch.- In the
!openPMD_USE_INVASIVE_TESTSstub branch,<catch2/catch_tostring.hpp>is included but no Catch2 is used; move the Catch2 includes into the#elsebranch.♻️ Proposed include reorganization
-#include <catch2/catch_tostring.hpp> - -#if !openPMD_USE_INVASIVE_TESTS +#if !openPMD_USE_INVASIVE_TESTS namespace read_nonexistent_attribute { void read_nonexistent_attribute() {} } // namespace read_nonexistent_attribute `#else` `#define` OPENPMD_private public: `#define` OPENPMD_protected public: `#include` "CoreTests.hpp" `#include` "openPMD/Error.hpp" `#include` "openPMD/IO/AbstractIOHandler.hpp" `#include` "openPMD/IO/IOTask.hpp" `#include` <catch2/catch_test_macros.hpp> +#include <catch2/catch_tostring.hpp> + +#include <algorithm> +#include <string> +#include <vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Files_Core/read_nonexistent_attribute.cpp` around lines 23 - 63, The test is missing direct algorithm/vector/string includes and has a stray Catch include in the non-invasive stub; add `#include` <algorithm>, `#include` <vector>, and `#include` <string> inside the invasive branch (before testedFileExtensions and where std::remove_if is used) and move the Catch2 includes (e.g., <catch2/catch_test_macros.hpp> and <catch2/catch_tostring.hpp>) into the `#else` invasive branch so the stub branch (the empty read_nonexistent_attribute function) no longer includes Catch headers unnecessarily.src/RecordComponent.cpp (2)
978-998: Remove the stale commented-outstatic_assert.The
// static_assert(... "EVIL")comment looks like leftover debug scaffolding and makes this otherwise clean forwarding wrapper noisier.Proposed cleanup
template <typename T> void RecordComponent::loadChunk(std::shared_ptr<T> data, Offset o, Extent e) { - // static_assert(!std::is_same_v<T_with_extent, std::string>, "EVIL"); uint8_t dim = getDimensionality();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RecordComponent.cpp` around lines 978 - 998, Remove the stale commented-out static_assert in RecordComponent::loadChunk: locate the line "// static_assert(!std::is_same_v<T_with_extent, std::string>, "EVIL");" within the RecordComponent::loadChunk function and delete that commented debug scaffold so the forwarding wrapper remains clean and free of leftover debug comments.
38-39: Remove the accidental duplicate macro include.
DatatypeMacros.hppis already included on Line 36; the added placeholder comment and second include just add macro-redefinition noise.Proposed cleanup
-// comment -#include "openPMD/DatatypeMacros.hpp" -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RecordComponent.cpp` around lines 38 - 39, The file contains a duplicate include of the same header "openPMD/DatatypeMacros.hpp" (the second include and its placeholder comment around the added line) which causes macro-redefinition noise; remove the accidental second include and the placeholder comment so only the original include (the one already present earlier in the file) remains, leaving a single `#include` "openPMD/DatatypeMacros.hpp" in RecordComponent.cpp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/openPMD/auxiliary/Defer.hpp`:
- Around line 39-51: The to_opaque() && implementation currently zeroes
do_run_this unconditionally, so it always returns a no-op defer and drops
functor; fix it by preserving the current do_run_this state, only moving functor
and clearing do_run_this when it was true. Concretely: in to_opaque() && read
do_run_this into a local flag, if that flag is true move functor into the
returned defer_type<std::function<void()>> and set do_run_this = false;
otherwise return an empty defer_type with do_run_this==false. Update references
to to_opaque(), defer_type, do_run_this, and functor accordingly so the else
branch is reachable and the moved functor is executed on destruction.
In `@include/openPMD/auxiliary/Future.hpp`:
- Around line 3-4: The header uses std::conditional_t and std::is_void_v (used
in the Future type traits around the conditional return-type logic), but
<type_traits> is not included; add a direct `#include` <type_traits> alongside the
existing `#includes` in include/openPMD/auxiliary/Future.hpp so the public header
does not rely on transitive includes and compiles when used in isolation.
- Around line 36-95: The class DeferredComputation currently declares an
explicit destructor which causes an implicit copy constructor to be generated
and suppresses implicit moves; make the type move-only: remove or change the
explicit ~DeferredComputation() to a defaulted destructor, delete the copy
constructor and copy assignment (DeferredComputation(const DeferredComputation&)
= delete; DeferredComputation& operator=(const DeferredComputation&) = delete;),
and explicitly default the move constructor and move assignment
(DeferredComputation(DeferredComputation&&) noexcept = default;
DeferredComputation& operator=(DeferredComputation&&) noexcept = default;),
preserving the rvalue-qualified forget() semantics and one-time invocation
guarantee.
In `@include/openPMD/auxiliary/Memory.hpp`:
- Around line 79-90: The template overloads for WriteBuffer (the template
constructor template<typename T> explicit WriteBuffer(std::shared_ptr<T> ptr)
and the template assignment WriteBuffer &operator=(std::shared_ptr<T> const
&ptr)) are declared in Memory.hpp but defined only in Memory.cpp causing link
failures for types outside the explicit instantiations; move their definitions
into the header (Memory.hpp) or a header-included .tpp and mark them inline so
any T can be instantiated, or alternatively constrain these templates (via
SFINAE/concepts) to only the supported OPENPMD_FOREACH_DATASET_DATATYPE plus
void/void const. Locate the template constructor and operator= declarations in
class WriteBuffer and either place their full implementations inline in the
header or add a Memory.tpp included at the end of Memory.hpp (or add appropriate
type constraints) to resolve missing-link errors.
In `@include/openPMD/IO/InvalidatableFile.hpp`:
- Around line 91-99: Summary: The specializations for
std::hash<openPMD::InvalidatableFile> and std::less<openPMD::InvalidatableFile>
use deprecated member typedefs and miss <functional>; update them for C++20.
Fix: add `#include` <functional> and remove deprecated typedefs (argument_type,
result_type, first_argument_type, second_argument_type); change the hash
specialization to use a concrete return type size_t and an operator signature
like size_t operator()(openPMD::InvalidatableFile const&) const; and change the
less specialization to return bool with signature bool
operator()(openPMD::InvalidatableFile const&, openPMD::InvalidatableFile const&)
const;. Reference: std::hash<openPMD::InvalidatableFile> and struct
less<openPMD::InvalidatableFile> / their operator() declarations.
In `@include/openPMD/Iteration.hpp`:
- Around line 132-140: The member m_iterationIndex in class Iteration is
incorrectly default-initialized to 0 (engaging the optional) — change its
default to an empty optional (std::nullopt) so missing cached indices are
represented correctly; update the declaration of std::optional<uint64_t>
m_iterationIndex to use = std::nullopt and ensure any code relying on the
accessor that throws error::Internal (the Iteration accessor used by
Series::indexOf()) continues to detect and handle the empty state.
In `@src/binding/python/PatchRecordComponent.cpp`:
- Around line 132-136: Update the error thrown in the DT::BOOL case of the
lambda bound to Patch_Record_Component.store to use the "store: ..." prefix
instead of "make_constant: ...", and remove the unreachable `break;` after the
`throw`; locate the DT::BOOL branch in PatchRecordComponent.cpp within the
lambda bound to Patch_Record_Component.store(idx, data) and change the
runtime_error message to "store: Boolean type not supported!" (or similar
"store: ..." wording) and delete the trailing break statement.
In `@src/binding/python/RecordComponent.cpp`:
- Around line 492-499: The comparison using py::dtype.is() is performing
identity checks and can fail for many dtype variants; change the check in the
store path to use value-equality (replace
dtype_to_numpy(r.getDatatype()).is(a.dtype()) with
dtype_to_numpy(r.getDatatype()) == a.dtype()) so dtype_to_numpy,
r.getDatatype(), and a.dtype() are compared by value, and make the same
replacement in the analogous dtype comparison elsewhere in RecordComponent.cpp
(the second occurrence near the other store/convert code).
In `@src/IO/AbstractIOHandler.cpp`:
- Around line 125-139: The current AbstractIOHandler::flush uses m_work.empty()
before/after calling flush_impl to decide whether to increment *m_flushCounter;
if flush_impl can re-enqueue work this will prevent the counter from advancing
even though originally queued tasks were processed. Fix by either (A)
documenting and enforcing the invariant: add a clear comment in
AbstractIOHandler::flush and an assertion (or post-condition) that flush_impl
must drain all work it observes and must not leave newly-enqueued tasks behind
(reference m_work and flush_impl), or (B) make the counter task-level: capture a
generation/token for the work present before calling flush_impl and only advance
m_flushCounter when that generation’s tasks are completed (implement a sentinel
task or per-task completion marker rather than relying on m_work.empty()).
Ensure the chosen approach updates AbstractIOHandler::flush and any related
flush_impl implementations and mentions m_flushCounter, m_work, and flush_impl
in the change.
In `@src/IO/HDF5/HDF5IOHandler.cpp`:
- Around line 2022-2030: The defer_close_dataType cleanup uses auxiliary::defer
to call H5Tclose(dataType) but incorrectly treats status == 0 as an error;
change the condition to check for non-zero status (status != 0) before logging
the internal error so it matches other H5 close checks. Locate the lambda
capturing status/dataType inside defer_close_dataType and invert the if
condition that currently logs on status == 0 to instead log when H5Tclose
returns a non-zero status.
In `@src/IO/InvalidatableFile.cpp`:
- Around line 83-88: The comparator
std::less<openPMD::InvalidatableFile>::operator() currently orders by the
mutable FileState::name which can change after insertion and break
ADIOS2IOHandlerImpl's std::set (m_dirty); update the comparator used for
InvalidatableFile to rely on an immutable identity instead (for example compare
the address/owner pointer or a stable unique id of the underlying file object
rather than FileState::name) so ordering can't change when name mutates, or
alternatively ensure ADIOS2IOHandlerImpl erases and reinserts entries in m_dirty
whenever FileState::name is modified; touch the comparator implementation
(std::less<openPMD::InvalidatableFile>::operator()) and/or ADIOS2IOHandlerImpl
mutation paths to apply one of these fixes.
In `@src/LoadStoreChunk.cpp`:
- Around line 379-389: The explicit instantiations must include the template
arguments because they cannot be deduced; change the instantiation macros so
ConfigureLoadStore::load is emitted as ConfigureLoadStore::load<dtype>() and
ConfigureLoadStore::storeSpan is emitted as
ConfigureLoadStore::storeSpan<type>() (i.e., update INSTANTIATE_METHOD_TEMPLATES
to instantiate load<dtype>() and
INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT to instantiate
storeSpan<type>() for the plain type while retaining the existing instantiations
for OPENPMD_ARRAY(type)).
In `@src/RecordComponent.cpp`:
- Around line 243-258: Calculate and validate the total byte size before
allocating: after computing numPoints from extent `e`, ensure the extent is sane
and check for multiplication overflow against `dtype_size` (e.g., ensure
numPoints <= SIZE_MAX / dtype_size and that extents are positive/non-zero as
expected); perform any bounds/dimensionality validation on the prepareLoadStore
chain (the same checks that load() would perform) before creating `newData` so
you never allocate based on an invalid/overflowed size; if validation fails,
return or throw instead of allocating and then calling
withSharedPtr_impl_mut(...).load().
In `@test/python/unittest/API/APITest.py`:
- Around line 2346-2359: The new keepalive tests call NumPy via helpers like
backend_keepalive_mesh_component, backend_keepalive_particle_position, and
backend_keepalive_particle_patches but do not guard against NumPy being absent;
update each test (testKeepaliveMeshComponent, testKeepaliveParticlePosition,
testKeepaliveParticlePatches) to skip or return early when found_numpy is False
(or otherwise check the module-level found_numpy flag) so they don't call np and
raise NameError; ensure the same guard is applied to the other keepalive tests
in the 2361–2501 region that use np.
- Around line 2460-2501: The detached scalar particle-patch returned by
get_component_only (num_particles_comp) loses the underlying Series/Iteration
lifetime so component.store(...) + component.series_flush() can be dropped by
GC; fix by keeping the parent objects alive when returning the component (either
return the Series/Iteration alongside num_particles_comp or attach a keepalive
reference on the component to the Series/Iteration object returned by
get_component_only) so that num_particles_comp.store and
num_particles_comp.series_flush operate on a live backend; locate
get_component_only, num_particles_comp, component.store and
component.series_flush to implement this change (or, if the Python binding is
intended to own lifetime, update the binding lifetime policy for
particle_patches extraction to hold the parent reference).
---
Outside diff comments:
In `@include/openPMD/backend/Attributable.hpp`:
- Around line 30-36: The header uses uint8_t (as the underlying type for
EnqueueAsynchronously) but doesn't include <cstdint>, which risks relying on
transitive includes; add `#include` <cstdint> to
include/openPMD/backend/Attributable.hpp (alongside the existing includes) so
uint8_t is defined before the declaration/definition of EnqueueAsynchronously
and any code referencing it (search for the EnqueueAsynchronously symbol to
verify placement).
In `@include/openPMD/IO/DummyIOHandler.hpp`:
- Around line 43-48: The override of the internal hook flush_impl should not be
public: change the declaration of flush_impl(internal::ParsedFlushParams &)
override from public to protected in DummyIOHandler (and do the same in
HDF5IOHandler, JSONIOHandler, ParallelHDF5IOHandler) so callers cannot bypass
the public flush() wrapper; keep the signature and the override specifier
unchanged to preserve the customization point declared protected in
AbstractIOHandler.
In `@src/auxiliary/Filesystem.cpp`:
- Around line 119-169: get_permissions currently returns 0 when get_parent(path)
yields an empty string, which causes create_directories/mk to lose sticky/setgid
bits for relative single-component paths like "foo"; change get_permissions (or
its caller) so that if parent is empty it uses the current directory (e.g. treat
parent as "." ) before checking directory_exists or calling stat, so
get_permissions can return the actual parent mode bits; update references in
get_permissions/get_parent/create_directories so mk() preserves S_ISVTX|S_ISGID
from "." for relative paths.
- Around line 107-110: The function list_directory_nothrow currently iterates
with readdir and can return a partial listing because readdir returns nullptr on
both end-of-dir and error; fix it by clearing errno before the loop (errno = 0),
using while ((entry = readdir(directory)) != nullptr) to collect names into ret,
then after the loop check if errno != 0 to detect an error; on error, ensure you
close the DIR* (closedir(directory)), clear/ignore ret and return std::nullopt,
otherwise close directory and return ret as before—apply this change around the
existing readdir loop and closedir handling in list_directory_nothrow.
- Around line 90-99: The Windows branch violates the nothrow contract by
throwing on FindFirstFile failure and misuses errno; update the logic in the
routine that enumerates directory entries (the block using
FindFirstFile/FindNextFile) to return std::nullopt instead of throwing when
FindFirstFile returns INVALID_HANDLE_VALUE, use GetLastError() to capture Win32
error codes (not errno), and after the FindNextFile loop check GetLastError() to
distinguish ERROR_NO_MORE_FILES from real failures; ensure FindClose(hFind) is
always called on a valid handle before returning and surface a std::nullopt on
any real Win32 error.
In `@src/binding/python/Series.cpp`:
- Around line 502-508: The comment on the py::cpp_function exposing
Series::iterations is stale for py::keep_alive<0, 1>(); update it to accurately
state the semantics of py::keep_alive<0, 1>() (i.e., the Series object (patient
1) is kept alive for as long as the returned iterations object (nurse 0) lives)
and remove or replace the existing "garbage collection: return value must be
freed before Series" wording; reference the Series class, the iterations
property, and py::keep_alive<0, 1>() when editing.
- Around line 618-663: The __del__ destructor is currently being chained onto
the module binding (m.def(...)) so it's bound at module level instead of to the
Series class; move the .def("__del__", ...) call so it is invoked on the class
binding object (cl) that defines Series (i.e., attach the lambda that calls
s.close() and swallows exceptions to cl via cl.def("__del__", ...)), ensuring
the destructor is registered on the Series class rather than on the module.
In `@src/IO/ADIOS/ADIOS2IOHandler.cpp`:
- Around line 1300-1325: The current getBufferView implementation sets
parameters.out->backendManagedBuffer = true when parameters.queryOnly is true
even for engines that will later be rejected by the real span-eligibility checks
(optInEngines, BP5/version auto-disable, UseSpan::No), causing inconsistent
responses; update the logic in ADIOS2IOHandlerImpl::getBufferView so that
parameters.queryOnly only causes backendManagedBuffer = true after the same full
eligibility checks used for the real getBufferView call succeed (i.e., verify
realEngineType() is in optInEngines and run the BP5/version and UseSpan::No
checks before honoring parameters.queryOnly), or alternatively compute a single
boolean like canUseSpan (based on optInEngines, BP5/version handling, and
UseSpan::No) and use that both for the queryOnly branch and the real call so
backendManagedBuffer is consistent.
In `@src/IO/HDF5/HDF5IOHandler.cpp`:
- Around line 2565-2615: The defer guard currently captures dataType by
reference so when dataType is reassigned to member handles
(m_H5T_LONG_DOUBLE_80_LE / m_H5T_CLONG_DOUBLE_80_LE) the guard will close those
members later; to fix, capture and close the original dataset type handle only
by storing it in a local variable (e.g. origDataType = dataType) before any
potential reassignment and update the defer_close_dataType to close origDataType
(or capture by value), leaving dataType free to be reassigned after the check
with checkDatasetTypeAgain (use H5Dget_type and H5Tequal as already present) so
member handles are not closed by the local defer.
- Around line 1528-1658: The loop incorrectly installs a defer that closes
next_type then assigns next_type to dataset_type (causing use-after-close) and
also never closes the old dataset_type (leak). Fix by removing the defer that
closes next_type inside the else branch; instead, before reassigning, call
H5Tclose(dataset_type) to close the current dataset_type handle, then set
dataset_type = next_type and ensure the existing defer_close_dataset_type (or a
single auxiliary::defer managing dataset_type) will close the new dataset_type
at scope exit; keep references to H5Tget_super, H5Tclose, H5Tequal,
dataset_type, next_type, and defer_close_dataset_type to locate and adjust the
logic.
---
Nitpick comments:
In `@include/openPMD/backend/PatchRecordComponent.hpp`:
- Around line 121-214: Update the error messages in
PatchRecordComponent::store(uint64_t idx, T) and PatchRecordComponent::store(T)
to reflect the new isSame semantics: replace "Datatypes of patch data (...) and
dataset (...) do not match." with wording that indicates representational
compatibility (e.g. "Datatypes of patch data (...) and dataset (...) are not
representation-compatible.") so the message matches that isSame allows
aliased-but-equivalent enum values; keep the existing dtype/getDatatype()
details in the message.
In `@include/openPMD/Dataset.hpp`:
- Around line 37-47: Add a short invariant note and a lightweight validation
helper for MemorySelection: document that MemorySelection::offset.size() must
equal MemorySelection::extent.size() and that both must match the dimensionality
of the target dataset/chunk, and add a small member function bool valid() const
(or a constructor overload) on struct MemorySelection that returns false if
offset.size() != extent.size() (and optionally checks for non-negative
extents/offsets); update the struct comment to state the invariant and reference
MemorySelection::valid() so callers can perform an early check before passing to
backends like ADIOS2.
In `@include/openPMD/IO/AbstractIOHandler.hpp`:
- Around line 267-283: Move the public m_flushCounter member into the non-public
bookkeeping area of AbstractIOHandler (near m_lastFlushSuccessful and m_work)
and change its visibility to protected so subclasses can access it but external
callers cannot reset/reassign it; ensure the existing initialization
(std::make_shared<unsigned long long>(0)) stays intact and that the two
overloads flush(internal::FlushParams) and flush(internal::ParsedFlushParams&)
still reference the member correctly after the move.
In `@include/openPMD/IO/ADIOS/macros.hpp`:
- Around line 52-69: Include the proper standard headers (<utility> and
<type_traits>), change the SFINAE specialization in
detail::CanTheMemorySelectionBeReset to use std::void_t to detect the presence
of a SetMemorySelection member regardless of its return type (e.g. use
template<typename V, typename =
std::void_t<decltype(std::declval<V>().SetMemorySelection())>>), and make the
top-level CanTheMemorySelectionBeReset a templated constexpr variable template
(instead of hardcoding adios2::Variable<int>) so callers can query
CanTheMemorySelectionBeReset<SomeVarType>.
In `@src/RecordComponent.cpp`:
- Around line 978-998: Remove the stale commented-out static_assert in
RecordComponent::loadChunk: locate the line "//
static_assert(!std::is_same_v<T_with_extent, std::string>, "EVIL");" within the
RecordComponent::loadChunk function and delete that commented debug scaffold so
the forwarding wrapper remains clean and free of leftover debug comments.
- Around line 38-39: The file contains a duplicate include of the same header
"openPMD/DatatypeMacros.hpp" (the second include and its placeholder comment
around the added line) which causes macro-redefinition noise; remove the
accidental second include and the placeholder comment so only the original
include (the one already present earlier in the file) remains, leaving a single
`#include` "openPMD/DatatypeMacros.hpp" in RecordComponent.cpp.
In `@test/Files_Core/read_nonexistent_attribute.cpp`:
- Around line 23-63: The test is missing direct algorithm/vector/string includes
and has a stray Catch include in the non-invasive stub; add `#include`
<algorithm>, `#include` <vector>, and `#include` <string> inside the invasive branch
(before testedFileExtensions and where std::remove_if is used) and move the
Catch2 includes (e.g., <catch2/catch_test_macros.hpp> and
<catch2/catch_tostring.hpp>) into the `#else` invasive branch so the stub branch
(the empty read_nonexistent_attribute function) no longer includes Catch headers
unnecessarily.
In `@test/ParallelIOTest.cpp`:
- Around line 538-562: The test unconditionally issues a load for last_row via
E_y.prepareLoadStore().offset({4, 0}).load<int>().get() even when
CanTheMemorySelectionBeReset is false and that row was never written; guard the
load behind the same constexpr condition to avoid pulling unwritten data. Modify
the block so that last_row is only constructed/loaded when
CanTheMemorySelectionBeReset is true (matching the later assertion loop),
keeping the existing checks that iterate over {&first_row, &last_row} when true
and only &first_row when false. Ensure references to last_row in the subsequent
loops are only present when the constexpr branch includes it (use the same
lambda/return pattern already used).
In `@test/SerialIOTest.cpp`:
- Around line 1760-1770: The test currently only prints the first loaded value
for variantTypeDataset and variantTypeDataset2; add assertions that verify the
loaded variant's first element equals 0.0 for both load paths instead of just
printing. Locate the loop over {&variantTypeDataset, &variantTypeDataset2} and
inside the std::visit lambda (used with prepareLoadStore().loadVariant().get()
results after rc.seriesFlush()) replace or augment the std::cout with an
assertion (e.g., ASSERT_DOUBLE_EQ/EXPECT_DOUBLE_EQ or appropriate test macro)
that checks shared_ptr.get()[0] == 0.0 for every visited variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 939afba6-6b71-46ad-92b7-53c86e9f860a
📒 Files selected for processing (66)
.pre-commit-config.yamlCMakeLists.txtinclude/openPMD/Dataset.hppinclude/openPMD/Datatype.hppinclude/openPMD/Datatype.tppinclude/openPMD/IO/ADIOS/ADIOS2File.hppinclude/openPMD/IO/ADIOS/ADIOS2IOHandler.hppinclude/openPMD/IO/ADIOS/macros.hppinclude/openPMD/IO/AbstractIOHandler.hppinclude/openPMD/IO/AbstractIOHandlerImplCommon.hppinclude/openPMD/IO/DummyIOHandler.hppinclude/openPMD/IO/HDF5/HDF5IOHandler.hppinclude/openPMD/IO/HDF5/ParallelHDF5IOHandler.hppinclude/openPMD/IO/IOTask.hppinclude/openPMD/IO/InvalidatableFile.hppinclude/openPMD/IO/JSON/JSONIOHandler.hppinclude/openPMD/Iteration.hppinclude/openPMD/LoadStoreChunk.hppinclude/openPMD/LoadStoreChunk.tppinclude/openPMD/ParticleSpecies.hppinclude/openPMD/RecordComponent.hppinclude/openPMD/RecordComponent.tppinclude/openPMD/auxiliary/Defer.hppinclude/openPMD/auxiliary/Filesystem.hppinclude/openPMD/auxiliary/Future.hppinclude/openPMD/auxiliary/Memory.hppinclude/openPMD/auxiliary/Memory_internal.hppinclude/openPMD/auxiliary/UniquePtr.hppinclude/openPMD/backend/Attributable.hppinclude/openPMD/backend/Attribute.hppinclude/openPMD/backend/ContainerImpl.tppinclude/openPMD/backend/PatchRecordComponent.hppsrc/Datatype.cppsrc/IO/ADIOS/ADIOS2File.cppsrc/IO/ADIOS/ADIOS2IOHandler.cppsrc/IO/ADIOS/ADIOS2PreloadAttributes.cppsrc/IO/AbstractIOHandler.cppsrc/IO/AbstractIOHandlerImpl.cppsrc/IO/DummyIOHandler.cppsrc/IO/HDF5/HDF5IOHandler.cppsrc/IO/HDF5/ParallelHDF5IOHandler.cppsrc/IO/InvalidatableFile.cppsrc/IO/JSON/JSONIOHandler.cppsrc/IO/JSON/JSONIOHandlerImpl.cppsrc/Iteration.cppsrc/LoadStoreChunk.cppsrc/RecordComponent.cppsrc/Series.cppsrc/auxiliary/Filesystem.cppsrc/auxiliary/Future.cppsrc/auxiliary/Memory.cppsrc/auxiliary/UniquePtr.cppsrc/backend/Attributable.cppsrc/backend/BaseRecord.cppsrc/binding/python/Attributable.cppsrc/binding/python/Iteration.cppsrc/binding/python/ParticleSpecies.cppsrc/binding/python/PatchRecordComponent.cppsrc/binding/python/RecordComponent.cppsrc/binding/python/Series.cpptest/CoreTest.cpptest/Files_Core/CoreTests.hpptest/Files_Core/read_nonexistent_attribute.cpptest/ParallelIOTest.cpptest/SerialIOTest.cpptest/python/unittest/API/APITest.py
💤 Files with no reviewable changes (1)
- src/Datatype.cpp
Comment on lines
+83
to
+88
| auto std::less<openPMD::InvalidatableFile>::operator()( | ||
| first_argument_type const &first, second_argument_type const &second) const | ||
| -> result_type | ||
| { | ||
| return less<>()(*first, *second); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect m_dirty lifecycle and InvalidatableFile name mutation paths.
# Expected: Any path that mutates FileState::name for an InvalidatableFile that may
# be present in m_dirty should erase/reinsert or avoid using InvalidatableFile as
# the ordered key.
printf '\n== m_dirty usage ==\n'
rg -n -C3 '\bm_dirty\b'
printf '\n== InvalidatableFile name mutation APIs ==\n'
rg -n -C3 'InvalidatableFile::operator=\(std::string|std::string\s*&\s*openPMD::InvalidatableFile::operator\*\(|std::string\s*\*\s*openPMD::InvalidatableFile::operator->\('
printf '\n== Direct InvalidatableFile invalidation calls ==\n'
rg -n -C3 '\.invalidate\s*\('Repository: franzpoeschel/openPMD-api
Length of output: 14741
🏁 Script executed:
# Check which handlers override m_dirty and what container types they use
rg -n 'std::.*set.*InvalidatableFile|std::.*set.*File.*m_dirty' include/openPMD/IO/ src/IO/
# Check the File typedef
rg -n 'using File|typedef.*File' include/openPMD/IO/AbstractIOHandlerImplCommon.hpp src/IO/InvalidatableFile.hpp
# Look for ADIOS2IOHandler's m_dirty handling
rg -n 'class ADIOS2IOHandlerImpl' include/openPMD/IO/ADIOS/ADIOS2IOHandler.hppRepository: franzpoeschel/openPMD-api
Length of output: 408
🏁 Script executed:
# Check if files are modified via operator* or operator-> while potentially in m_dirty
# Look for patterns like: *file = ..., file->... = ... on files that might be in m_dirty
rg -n '\*\w+\s*=' src/IO/ADIOS/ADIOS2IOHandler.cpp src/IO/JSON/JSONIOHandlerImpl.cpp | head -30
rg -n '->\w+\s*=' src/IO/ADIOS/ADIOS2IOHandler.cpp src/IO/JSON/JSONIOHandlerImpl.cpp | head -30
# Check for reassignments via operator=
rg -n 'file\s*=' src/IO/ADIOS/ADIOS2IOHandler.cpp src/IO/JSON/JSONIOHandlerImpl.cpp | grep -v '==' | head -20Repository: franzpoeschel/openPMD-api
Length of output: 4359
🏁 Script executed:
# Check base class to see if ADIOS2IOHandlerImpl inherits the ordered set
head -100 include/openPMD/IO/ADIOS/ADIOS2IOHandler.hpp | grep -A5 'class ADIOS2IOHandlerImpl'
grep -A20 'class ADIOS2IOHandlerImpl' include/openPMD/IO/ADIOS/ADIOS2IOHandler.hpp | head -30Repository: franzpoeschel/openPMD-api
Length of output: 1080
🏁 Script executed:
# Find the File typedef
rg -n 'using File|typedef.*File' include/openPMD/IO/AbstractIOHandlerImplCommon.hpp
# Check if there are other handlers besides JSON and ADIOS2
find include/openPMD/IO -name "*Handler*" -type f | head -20
rg -n 'class.*IOHandlerImpl.*AbstractIOHandlerImplCommon' include/openPMD/IO/
# Check if any files in m_dirty are ever modified via their mutable operators
# Look for: *file being assigned or mutated after being added to m_dirty
rg -n 'operator\*\(\)' src/IO/Repository: franzpoeschel/openPMD-api
Length of output: 706
🏁 Script executed:
# Look at InvalidatableFile.cpp to understand the comparator
cat src/IO/InvalidatableFile.cppRepository: franzpoeschel/openPMD-api
Length of output: 2524
🏁 Script executed:
# Check HDF5 handler classes - do they inherit or override m_dirty?
rg -n 'class HDF5IOHandlerImpl|m_dirty' include/openPMD/IO/HDF5/HDF5IOHandlerImpl.hpp
# Look at actual handler implementation hierarchy
head -50 include/openPMD/IO/HDF5/HDF5IOHandlerImpl.hpp | grep -A5 'class HDF5IOHandlerImpl'
# Check DummyIOHandler
rg -n 'class DummyIOHandler|m_dirty' include/openPMD/IO/DummyIOHandler.hppRepository: franzpoeschel/openPMD-api
Length of output: 343
🏁 Script executed:
# Critical: Look for patterns where InvalidatableFile objects are reassigned/mutated
# after being inserted into m_dirty. Check ADIOS2 specifically.
# Look for file = ... patterns or *file = ... patterns after m_dirty.emplace
rg -B5 -A10 'm_dirty\.emplace' src/IO/ADIOS/ADIOS2IOHandler.cpp | grep -A10 -B10 'file.*='Repository: franzpoeschel/openPMD-api
Length of output: 2817
🏁 Script executed:
# Check whether files accessed via operator* or operator-> after being in m_dirty
# Look at specific locations where files from m_dirty might be modified
sed -n '699,710p' src/IO/ADIOS/ADIOS2IOHandler.cpp
sed -n '2310,2330p' src/IO/ADIOS/ADIOS2IOHandler.cppRepository: franzpoeschel/openPMD-api
Length of output: 1481
🏁 Script executed:
# Check HDF5 handler hierarchy more carefully
grep -n "class HDF5IOHandlerImpl\|AbstractIOHandlerImpl" include/openPMD/IO/HDF5/HDF5IOHandlerImpl.hpp | head -20
# Check if AbstractIOHandlerImpl also declares m_dirty
rg -n 'm_dirty' include/openPMD/IO/AbstractIOHandlerImpl.hpp
# Look at the inheritance chain
rg -n 'class AbstractIOHandlerImpl' include/openPMD/IO/Repository: franzpoeschel/openPMD-api
Length of output: 400
🏁 Script executed:
# Critical: Search for any place where files might get aliased and then one alias is mutated
# Look for shared_ptr patterns with InvalidatableFile or File
rg -n 'shared_ptr.*InvalidatableFile|std::make_shared.*InvalidatableFile' include/openPMD/IO/ src/IO/
# Check if m_files or other maps might share the same FileState via aliases
sed -n '40,100p' include/openPMD/IO/AbstractIOHandlerImplCommon.hppRepository: franzpoeschel/openPMD-api
Length of output: 2232
🏁 Script executed:
# Check if HDF5 even has m_dirty - does it inherit from AbstractIOHandlerImplCommon or just AbstractIOHandlerImpl?
grep -n "class HDF5IOHandlerImpl\|AbstractIOHandlerImpl" include/openPMD/IO/HDF5/HDF5IOHandlerImpl.hpp
# Check AbstractIOHandlerImpl to see if it has m_dirty
rg -n 'm_dirty' include/openPMD/IO/AbstractIOHandlerImpl.hpp
# Now let's find all handlers and what they inherit from
rg -n 'class.*IOHandlerImpl\s*:' include/openPMD/IO/
# Critical: Look for actual file rename scenarios - search for operator= assignments on files
# that are already in m_dirty
rg -n 'InvalidatableFile.*operator=|InvalidatableFile &.*=' src/IO/ADIOS/ src/IO/JSON/ | head -20Repository: franzpoeschel/openPMD-api
Length of output: 530
🏁 Script executed:
# Check the actual File typedef in handlers
rg -n 'using File\s*=|typedef.*File' include/openPMD/IO/AbstractIOHandlerImplCommon.hpp include/openPMD/IO/JSON/JSONIOHandlerImpl.hpp
# Look at which handlers inherit from which base
cat include/openPMD/IO/JSON/JSONIOHandlerImpl.hpp | head -50 | grep -A5 "class JSONIOHandlerImpl"Repository: franzpoeschel/openPMD-api
Length of output: 159
🏁 Script executed:
# Check if AbstractIOHandlerImpl declares m_dirty
rg -n "m_dirty" include/openPMD/IO/AbstractIOHandlerImpl.hpp
# Understand the full class hierarchy
sed -n '30,60p' include/openPMD/IO/AbstractIOHandlerImpl.hpp
sed -n '1,50p' include/openPMD/IO/AbstractIOHandlerImplCommon.hppRepository: franzpoeschel/openPMD-api
Length of output: 2798
🏁 Script executed:
# Search for any scenario where a file object in m_dirty might be reassigned
# Look for patterns: for loops iterating m_dirty, then file = ..., or getting file from m_dirty then modifying
rg -B10 -A5 'for.*m_dirty|for.*dirty' src/IO/ADIOS/ src/IO/JSON/
# Also check if files are ever obtained from m_dirty and then mutated
rg -B5 'auto.*=.*m_dirty' src/IO/ADIOS/Repository: franzpoeschel/openPMD-api
Length of output: 2116
🏁 Script executed:
# Final verification: Check HDF5IOHandlerImpl to confirm it doesn't use m_dirty at all
grep -n "m_dirty\|m_files" include/openPMD/IO/HDF5/HDF5IOHandlerImpl.hpp | head -20
# Confirm inheritance chain one more time with explicit checks
rg -n 'class AbstractIOHandlerImplCommon|class ADIOS2IOHandlerImpl|class JSONIOHandlerImpl' include/openPMD/IO/Repository: franzpoeschel/openPMD-api
Length of output: 687
🏁 Script executed:
# Check if there are any actual rename operations on InvalidatableFile objects
# that are in m_dirty (look for patterns like: file = ..., *file = ..., etc.)
# in ADIOS2 handler which uses the ordered set
rg -B3 -A3 'operator=\(std::string' src/IO/ADIOS/ADIOS2IOHandler.cpp
# Look for any code that might mutate file names after adding to m_dirty
rg -n 'refreshFileFromParent|associateWithFile' src/IO/ADIOS/ADIOS2IOHandler.cpp | head -10Repository: franzpoeschel/openPMD-api
Length of output: 844
🏁 Script executed:
# Final check: confirm ADIOS2IOHandlerImpl inheritance
grep -A10 "class ADIOS2IOHandlerImpl" include/openPMD/IO/ADIOS/ADIOS2IOHandler.hpp | head -15Repository: franzpoeschel/openPMD-api
Length of output: 531
🏁 Script executed:
# Check the File typedef in JSONIOHandlerImpl
sed -n '120,170p' include/openPMD/IO/JSON/JSONIOHandlerImpl.hpp | grep -A5 -B5 "using File\|typedef.*File"Repository: franzpoeschel/openPMD-api
Length of output: 355
The comparator order depends on a mutable field, affecting only ADIOS2IOHandlerImpl's ordered set.
This comparator orders std::set<InvalidatableFile> entries by FileState::name, which is mutable through shared InvalidatableFile aliases via operator=(), operator*(), and operator->(). Only ADIOS2IOHandlerImpl uses the ordered std::set<InvalidatableFile> from AbstractIOHandlerImplCommon; JSONIOHandlerImpl overrides with std::unordered_set<File> and is unaffected. If a file's name is mutated after insertion into ADIOS2's m_dirty, the set's ordering invariants could break, causing lookup/erase failures. Prefer comparing by immutable file identity (pointer/hash) or ensure files are erased/reinserted on name changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/IO/InvalidatableFile.cpp` around lines 83 - 88, The comparator
std::less<openPMD::InvalidatableFile>::operator() currently orders by the
mutable FileState::name which can change after insertion and break
ADIOS2IOHandlerImpl's std::set (m_dirty); update the comparator used for
InvalidatableFile to rely on an immutable identity instead (for example compare
the address/owner pointer or a stable unique id of the underlying file object
rather than FileState::name) so ordering can't change when name mutates, or
alternatively ensure ADIOS2IOHandlerImpl erases and reinserts entries in m_dirty
whenever FileState::name is modified; touch the comparator implementation
(std::less<openPMD::InvalidatableFile>::operator()) and/or ADIOS2IOHandlerImpl
mutation paths to apply one of these fixes.
Comment on lines
+379
to
+389
| #define INSTANTIATE_METHOD_TEMPLATES(dtype) \ | ||
| template auto ConfigureLoadStore::load() \ | ||
| -> auxiliary::DeferredComputation<OPENPMD_APPLY_TEMPLATE( \ | ||
| std::shared_ptr, dtype)>; | ||
| #define INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT(type) \ | ||
| INSTANTIATE_METHOD_TEMPLATES(type) \ | ||
| INSTANTIATE_METHOD_TEMPLATES(OPENPMD_ARRAY(type)) \ | ||
| template auto ConfigureLoadStore::storeSpan() -> DynamicMemoryView<type>; | ||
|
|
||
| OPENPMD_FOREACH_DATASET_DATATYPE( | ||
| INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the old non-deducible explicit-instantiation pattern is gone.
rg -n -C2 'template auto ConfigureLoadStore::(load|storeSpan)\(\)' src/LoadStoreChunk.cppRepository: franzpoeschel/openPMD-api
Length of output: 668
Specify template arguments in explicit instantiations.
The template arguments cannot be deduced without function parameters. Lines 380 and 386 require explicit template argument specification: load<dtype>() and storeSpan<type>().
Proposed fix
`#define` INSTANTIATE_METHOD_TEMPLATES(dtype) \
- template auto ConfigureLoadStore::load() \
+ template auto ConfigureLoadStore::load<dtype>() \
-> auxiliary::DeferredComputation<OPENPMD_APPLY_TEMPLATE( \
std::shared_ptr, dtype)>;
`#define` INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT(type) \
INSTANTIATE_METHOD_TEMPLATES(type) \
INSTANTIATE_METHOD_TEMPLATES(OPENPMD_ARRAY(type)) \
- template auto ConfigureLoadStore::storeSpan() -> DynamicMemoryView<type>;
+ template auto ConfigureLoadStore::storeSpan<type>() \
+ -> DynamicMemoryView<type>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #define INSTANTIATE_METHOD_TEMPLATES(dtype) \ | |
| template auto ConfigureLoadStore::load() \ | |
| -> auxiliary::DeferredComputation<OPENPMD_APPLY_TEMPLATE( \ | |
| std::shared_ptr, dtype)>; | |
| #define INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT(type) \ | |
| INSTANTIATE_METHOD_TEMPLATES(type) \ | |
| INSTANTIATE_METHOD_TEMPLATES(OPENPMD_ARRAY(type)) \ | |
| template auto ConfigureLoadStore::storeSpan() -> DynamicMemoryView<type>; | |
| OPENPMD_FOREACH_DATASET_DATATYPE( | |
| INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT) | |
| `#define` INSTANTIATE_METHOD_TEMPLATES(dtype) \ | |
| template auto ConfigureLoadStore::load<dtype>() \ | |
| -> auxiliary::DeferredComputation<OPENPMD_APPLY_TEMPLATE( \ | |
| std::shared_ptr, dtype)>; | |
| `#define` INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT(type) \ | |
| INSTANTIATE_METHOD_TEMPLATES(type) \ | |
| INSTANTIATE_METHOD_TEMPLATES(OPENPMD_ARRAY(type)) \ | |
| template auto ConfigureLoadStore::storeSpan<type>() \ | |
| -> DynamicMemoryView<type>; | |
| OPENPMD_FOREACH_DATASET_DATATYPE( | |
| INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT) |
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 379-379: syntax error
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LoadStoreChunk.cpp` around lines 379 - 389, The explicit instantiations
must include the template arguments because they cannot be deduced; change the
instantiation macros so ConfigureLoadStore::load is emitted as
ConfigureLoadStore::load<dtype>() and ConfigureLoadStore::storeSpan is emitted
as ConfigureLoadStore::storeSpan<type>() (i.e., update
INSTANTIATE_METHOD_TEMPLATES to instantiate load<dtype>() and
INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT to instantiate
storeSpan<type>() for the plain type while retaining the existing instantiations
for OPENPMD_ARRAY(type)).
Comment on lines
+243
to
+258
| size_t numPoints = 1; | ||
| for (auto val : e) | ||
| { | ||
| numPoints *= val; | ||
| } | ||
|
|
||
| auto newData = | ||
| std::shared_ptr<void>(new char[numPoints * dtype_size], [](void *p) { | ||
| delete[] (static_cast<char *>(p)); | ||
| }); | ||
| prepareLoadStore() | ||
| .offset(std::move(o)) | ||
| .extent(std::move(e)) | ||
| .withSharedPtr_impl_mut(newData, dtype) | ||
| .unsafeNoAutomaticFlush() | ||
| .load(); |
There was a problem hiding this comment.
Validate the chunk and check allocation overflow before allocating.
numPoints * dtype_size can wrap, and this allocates before the later load path validates dimensionality/bounds. A huge or invalid extent can under-allocate or OOM before being rejected.
Proposed hardening
+#include <limits>
+
@@
- size_t numPoints = 1;
- for (auto val : e)
- {
- numPoints *= val;
- }
+ auto dim = getDimensionality();
+ if (e.size() != dim || o.size() != dim)
+ {
+ throw std::runtime_error("Dimensionality of chunk and record component do not match.");
+ }
+
+ Extent dse = getExtent();
+ for (uint8_t i = 0; i < dim; ++i)
+ {
+ if (o[i] > dse[i] || e[i] > dse[i] - o[i])
+ {
+ throw std::runtime_error("Chunk does not reside inside dataset.");
+ }
+ }
+
+ size_t numPoints = 1;
+ for (auto val : e)
+ {
+ if (val > std::numeric_limits<size_t>::max() / numPoints)
+ {
+ throw std::overflow_error("Chunk allocation size overflows size_t.");
+ }
+ numPoints *= static_cast<size_t>(val);
+ }
+
+ if (dtype_size != 0 &&
+ numPoints > std::numeric_limits<size_t>::max() / dtype_size)
+ {
+ throw std::overflow_error("Chunk allocation byte size overflows size_t.");
+ }
auto newData =
std::shared_ptr<void>(new char[numPoints * dtype_size], [](void *p) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/RecordComponent.cpp` around lines 243 - 258, Calculate and validate the
total byte size before allocating: after computing numPoints from extent `e`,
ensure the extent is sane and check for multiplication overflow against
`dtype_size` (e.g., ensure numPoints <= SIZE_MAX / dtype_size and that extents
are positive/non-zero as expected); perform any bounds/dimensionality validation
on the prepareLoadStore chain (the same checks that load() would perform) before
creating `newData` so you never allocate based on an invalid/overflowed size; if
validation fails, return or throw instead of allocating and then calling
withSharedPtr_impl_mut(...).load().
Comment on lines
+2346
to
+2359
| def testKeepaliveMeshComponent(self): | ||
| """Test keepalive for mesh component extraction.""" | ||
| for ext in tested_file_extensions: | ||
| self.backend_keepalive_mesh_component(ext) | ||
|
|
||
| def testKeepaliveParticlePosition(self): | ||
| """Test keepalive for particle position component extraction.""" | ||
| for ext in tested_file_extensions: | ||
| self.backend_keepalive_particle_position(ext) | ||
|
|
||
| def testKeepaliveParticlePatches(self): | ||
| """Test keepalive for particle patches component extraction.""" | ||
| for ext in tested_file_extensions: | ||
| self.backend_keepalive_particle_patches(ext) |
There was a problem hiding this comment.
Guard the new NumPy-dependent keepalive tests.
These helpers use np unconditionally. In a no-NumPy test environment, the module only sets found_numpy = False, so these tests will raise NameError instead of being skipped.
🧪 Proposed fix
def testKeepaliveMeshComponent(self):
"""Test keepalive for mesh component extraction."""
+ if not found_numpy:
+ return
for ext in tested_file_extensions:
self.backend_keepalive_mesh_component(ext)
def testKeepaliveParticlePosition(self):
"""Test keepalive for particle position component extraction."""
+ if not found_numpy:
+ return
for ext in tested_file_extensions:
self.backend_keepalive_particle_position(ext)
def testKeepaliveParticlePatches(self):
"""Test keepalive for particle patches component extraction."""
+ if not found_numpy:
+ return
for ext in tested_file_extensions:
self.backend_keepalive_particle_patches(ext)Also applies to: 2361-2501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/python/unittest/API/APITest.py` around lines 2346 - 2359, The new
keepalive tests call NumPy via helpers like backend_keepalive_mesh_component,
backend_keepalive_particle_position, and backend_keepalive_particle_patches but
do not guard against NumPy being absent; update each test
(testKeepaliveMeshComponent, testKeepaliveParticlePosition,
testKeepaliveParticlePatches) to skip or return early when found_numpy is False
(or otherwise check the module-level found_numpy flag) so they don't call np and
raise NameError; ensure the same guard is applied to the other keepalive tests
in the 2361–2501 region that use np.
Comment on lines
+2460
to
+2501
| def get_component_only(): | ||
| series = io.Series(path, io.Access.create_linear) | ||
| backend = series.backend | ||
| iteration = series.snapshots()[0] | ||
| particles = iteration.particles["electrons"] | ||
|
|
||
| dset = io.Dataset(np.dtype(np.float32), [30]) | ||
| position_x = particles["position"]["x"] | ||
| position_x.reset_dataset(dset) | ||
| position_x[:] = np.arange(30, dtype=np.float32) | ||
|
|
||
| dset = io.Dataset(np.dtype("uint64"), [2]) | ||
| num_particles_comp = particles.particle_patches["numParticles"] | ||
| num_particles_comp.reset_dataset(dset) | ||
| num_particles_comp.store(0, np.uint64(10)) | ||
| num_particles_comp.store(1, np.uint64(20)) | ||
|
|
||
| del iteration | ||
| del particles | ||
| del series | ||
| gc.collect() | ||
|
|
||
| return num_particles_comp, backend | ||
|
|
||
| component, backend = get_component_only() | ||
| gc.collect() | ||
|
|
||
| component.store(0, np.uint64(50)) | ||
|
|
||
| component.series_flush() | ||
| if backend == "ADIOS2": | ||
| del component | ||
| gc.collect() | ||
|
|
||
| read = io.Series(path, io.Access.read_only) | ||
| loaded = read.snapshots()[0] \ | ||
| .particles["electrons"].particle_patches["numParticles"].load() | ||
| read.flush() | ||
| np.testing.assert_array_equal( | ||
| loaded[0], | ||
| np.uint64(50) | ||
| ) |
There was a problem hiding this comment.
Fix the failing particle-patches keepalive path.
The macOS pipeline shows this test reads back 10 instead of the post-GC update 50. The detached object returned from get_component_only() is not keeping enough state alive for component.store(...); component.series_flush() to persist the change. If this test is meant to exercise the scalar patch component, extract the scalar component explicitly and fix the binding keepalive path if it still fails.
🧪 Proposed test-side correction
- num_particles_comp = particles.particle_patches["numParticles"]
+ num_particles_comp = particles.particle_patches[
+ "numParticles"
+ ][io.Record_Component.SCALAR]If this still reproduces 10, the root fix belongs in the Python binding lifetime policy for particle-patch component extraction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/python/unittest/API/APITest.py` around lines 2460 - 2501, The detached
scalar particle-patch returned by get_component_only (num_particles_comp) loses
the underlying Series/Iteration lifetime so component.store(...) +
component.series_flush() can be dropped by GC; fix by keeping the parent objects
alive when returning the component (either return the Series/Iteration alongside
num_particles_comp or attach a keepalive reference on the component to the
Series/Iteration object returned by get_component_only) so that
num_particles_comp.store and num_particles_comp.series_flush operate on a live
backend; locate get_component_only, num_particles_comp, component.store and
component.series_flush to implement this change (or, if the Python binding is
intended to own lifetime, update the binding lifetime policy for
particle_patches extraction to hold the parent reference).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| template <typename T> | ||
| void RecordComponent::loadChunk(std::shared_ptr<T> data, Offset o, Extent e) | ||
| void RecordComponent::loadChunk_impl( | ||
| std::shared_ptr<T> const &data, internal::LoadStoreConfigWithBuffer cfg) |
| std::static_pointer_cast<T>(std::move(ptr)), | ||
| std::move(offset), | ||
| std::move(extent)); | ||
| // static_assert(!std::is_same_v<T_with_extent, std::string>, "EVIL"); |
| automatic_variable_encoding::automatic_variable_encoding(); | ||
| } | ||
|
|
||
| TEST_CASE("read_nonexistent_attribute", "[core]") |
franzpoeschel
force-pushed
the
adios2-memory-selection
branch
from
02c635f to
f24964f
Compare
…enPMD#1873) this avoids subtle bugs followup to openPMD#1860
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/IO/HDF5/HDF5IOHandler.cpp (4)
1938-1991:⚠️ Potential issue | 🟠 Major
to_opaque()currently disables these memspace cleanup defers.Both
defer_close_memspace = auxiliary::defer(...).to_opaque()assignments rely oninclude/openPMD/auxiliary/Defer.hpp, but the provided implementation setsdo_run_this = falsebefore checking it, so the returned opaque defer is empty andH5Sclose(memspace)never runs.Fix the defer conversion helper
auto to_opaque() && -> defer_type<std::function<void()>> { - do_run_this = false; if (!do_run_this) { return defer_type<std::function<void()>>{{}, false}; } else { + do_run_this = false; return defer_type<std::function<void()>>{ std::function<void()>{std::move(functor)}}; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IO/HDF5/HDF5IOHandler.cpp` around lines 1938 - 1991, The opaque conversion helper to_opaque() is disabling the defer execution by setting do_run_this = false before wrapping; update to_opaque() in auxiliary::defer (used by defer_close_memspace) so the returned opaque defer retains the original "should run" flag and invokes the captured cleanup (H5Sclose(memspace)) when destroyed — i.e., implement the opaque wrapper to capture the original defer state and call the stored callable only if the original do_run_this was true, rather than clearing it, so the memspace cleanup in HDF5IOHandler (defer_close_memspace / H5Sclose) actually runs.
1528-1658:⚠️ Potential issue | 🔴 CriticalResource management bug: HDF5 type handle gets double-closed and reused after close.
In the retry loop (lines 142-162),
next_typefromH5Tget_super()is assigned todataset_type(line 158), thendefer_close_next_typeimmediately closes it when the else block exits. The next loop iteration uses the now-closeddataset_type, and the outerdefer_close_dataset_typelater closes it again. Additionally, the originalH5Dget_type()handle is lost if the loop repeats.Separate the original dataset type from the retry type. Keep a copy of the original
H5Dget_type()result and trackH5Tget_super()results for cleanup only after the loop completes, rather than closing them mid-iteration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IO/HDF5/HDF5IOHandler.cpp` around lines 1528 - 1658, The retry loop currently reassigns dataset_type (the original H5Dget_type() handle) to next_type and uses defer_close_next_type which closes next_type at end of the iteration, causing double-close and use-after-close; fix by keeping the original H5Dget_type() handle separate (e.g., keep original_type = dataset_type) and use a separate variable (e.g., probe_type) to walk H5Tget_super() results, collecting/closing only the probe_type handles after the loop ends; update references to defer_close_dataset_type to close original_type and remove/relocate defer_close_next_type so intermediate probe types are not closed mid-loop but cleaned up once the loop finishes or on error.
1188-1205:⚠️ Potential issue | 🟠 MajorClose
dataset_spaceandpropertyListon every exit path.
H5Dget_space()andH5Dget_create_plist()return copies that must be released withH5Sclose()andH5Pclose()respectively. Currently both leak on all paths: VERIFY() failure, exception fromH5Pget_chunk(), exception from thestd::runtime_error()throw, and normal scope exit.Proposed cleanup
{ hid_t dataset_space = H5Dget_space(dataset_id); + auto defer_close_dataset_space = auxiliary::defer([&]() { + status = H5Sclose(dataset_space); + if (status != 0) + { + std::cerr << "[HDF5] Internal error: Failed to close HDF5 " + "dataset space during dataset extension." + << std::endl; + } + }); int ndims = H5Sget_simple_extent_ndims(dataset_space); VERIFY( ndims >= 0, @@ hid_t propertyList = H5Dget_create_plist(dataset_id); + auto defer_close_propertyList = auxiliary::defer([&]() { + status = H5Pclose(propertyList); + if (status != 0) + { + std::cerr << "[HDF5] Internal error: Failed to close HDF5 " + "dataset creation property during dataset extension." + << std::endl; + } + }); std::vector<hsize_t> chunkExtent(ndims, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IO/HDF5/HDF5IOHandler.cpp` around lines 1188 - 1205, The dataset_space and propertyList handles returned by H5Dget_space() and H5Dget_create_plist() are leaked on every exit path; ensure you call H5Sclose(dataset_space) and H5Pclose(propertyList) on all paths (normal exit, after VERIFY failure, and when H5Pget_chunk() fails/throws). Wrap the acquisition in a small RAII or try/finally-style cleanup around the block that obtains dataset_space and propertyList (or explicitly close them before throwing), and keep the existing VERIFY and the runtime_error throw but make sure both handles are closed beforehand; reference the variables dataset_space, propertyList and the calls H5Dget_space, H5Dget_create_plist, H5Pget_chunk, H5Sclose, and H5Pclose when implementing the fix.
2565-2626:⚠️ Potential issue | 🔴 CriticalDo not let the close guard follow
dataTypeto member-owned HDF5 types.The
defer_close_dataTypelambda capturesdataTypeby reference. When the long-double fallback rewritesdataTypetom_H5T_LONG_DOUBLE_80_LEorm_H5T_CLONG_DOUBLE_80_LE(which are member-owned handles), the guard closes a member handle instead of the caller-owned copy. The destructor then attempts to close the same member handle again, causing undefined behavior.Capture
dataTypeby value to ensure the guard closes only the caller-owned copy, then use a separatereadDataTypevariable for the fallback assignments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IO/HDF5/HDF5IOHandler.cpp` around lines 2565 - 2626, The defer_close_dataType guard currently captures dataType by reference and may end up closing member-owned handles (m_H5T_LONG_DOUBLE_80_LE / m_H5T_CLONG_DOUBLE_80_LE) when dataType is reassigned for the long-double fallback; fix by making the defer_close_dataType capture the original caller-owned dataType by value and introduce a separate variable (e.g., readDataType) to hold the possibly-rewritten type used for reading; use dataType for the close guard and use readDataType in the H5Dread call and in the H5Tequal checks so member-owned handles are never closed by the local guard.src/RecordComponent.cpp (1)
787-793:⚠️ Potential issue | 🟠 MajorUse subtractive bounds checks to avoid unsigned wraparound.
offset + extentcan overflow and make an out-of-range chunk appear valid. Compareoffsetfirst, then compareextentagainst the remaining dataset size.Proposed bounds check
- if (dse[i] < o[i] + e[i]) + if (o[i] > dse[i] || e[i] > dse[i] - o[i]) throw std::runtime_error( "Chunk does not reside inside dataset (Dimension on " "index " + @@ - if (dse[i] < offset[i] + extent[i]) + if (offset[i] > dse[i] || extent[i] > dse[i] - offset[i]) throw std::runtime_error( "Chunk does not reside inside dataset (Dimension on index " + std::to_string(i) + ". DS: " + std::to_string(dse[i]) +Also applies to: 948-954
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RecordComponent.cpp` around lines 787 - 793, The current check uses dse[i] < o[i] + e[i] which can wrap on unsigned addition; change it to a subtractive check to avoid overflow by testing if the offset exceeds the dataset or the extent exceeds the remaining space: replace the condition with if (o[i] > dse[i] || e[i] > dse[i] - o[i]) and throw the same runtime_error (adjust message if desired); apply the same fix to the other occurrence that uses dse/o/e (the block around the later check referenced in the comment).
♻️ Duplicate comments (2)
src/LoadStoreChunk.cpp (1)
379-386:⚠️ Potential issue | 🔴 CriticalSpecify template arguments in the explicit instantiations.
load()andstoreSpan()have no function parameters, sodtype/typecannot be deduced here. This matches the static-analysis syntax error and will fail compilation.Proposed fix
`#define` INSTANTIATE_METHOD_TEMPLATES(dtype) \ - template auto ConfigureLoadStore::load() \ + template auto ConfigureLoadStore::load<dtype>() \ -> auxiliary::DeferredComputation<OPENPMD_APPLY_TEMPLATE( \ std::shared_ptr, dtype)>; `#define` INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT(type) \ INSTANTIATE_METHOD_TEMPLATES(type) \ INSTANTIATE_METHOD_TEMPLATES(OPENPMD_ARRAY(type)) \ - template auto ConfigureLoadStore::storeSpan() -> DynamicMemoryView<type>; + template auto ConfigureLoadStore::storeSpan<type>() -> DynamicMemoryView<type>;You can verify this location with:
#!/bin/bash # Description: Show explicit instantiations that still omit template arguments. # Expected result after the fix: no matches. rg -n -C2 'template auto ConfigureLoadStore::(load|storeSpan)\(\)' src/LoadStoreChunk.cpp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LoadStoreChunk.cpp` around lines 379 - 386, The explicit instantiations generated by INSTANTIATE_METHOD_TEMPLATES and INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT omit template argument syntax so the compiler can't deduce dtype/type for ConfigureLoadStore::load and ConfigureLoadStore::storeSpan; update the macros to emit explicit template argument forms (e.g. use "template auto ConfigureLoadStore::load<dtype>() -> ..." and "template auto ConfigureLoadStore::storeSpan<type>() -> DynamicMemoryView<type>;" and likewise for OPENPMD_ARRAY(type) variants) so the template parameter is specified in the instantiation for each dtype/type.src/RecordComponent.cpp (1)
238-258:⚠️ Potential issue | 🟠 MajorValidate before allocating and guard
numPoints * dtype_sizeoverflow.
numPointsand the final byte count can wrap beforenew char[...], and the actual load validation happens after allocation. Invalid or huge selections can under-allocate or OOM before being rejected.Suggested hardening shape
auto [o, e] = std::move(cfg); + verifyChunk(dtype, o, e); + size_t numPoints = 1; for (auto val : e) { + if (val > std::numeric_limits<size_t>::max() / numPoints) + { + throw std::overflow_error("Chunk allocation size overflows size_t."); + } numPoints *= val; } + if (dtype_size != 0 && + numPoints > std::numeric_limits<size_t>::max() / dtype_size) + { + throw std::overflow_error("Chunk allocation byte size overflows size_t."); + } auto newData =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RecordComponent.cpp` around lines 238 - 258, Before allocating the buffer in RecordComponent::loadChunkAllocate_impl, validate the computed selection and guard the multiplication numPoints * dtype_size for overflow: first call the same validation that prepareLoadStore() uses (or invoke prepareLoadStore().offset(...).extent(...).validate() or perform equivalent checks on o and e) to reject invalid/huge selections, then compute size_t bytes = numPoints; if (dtype_size != 0 && bytes > SIZE_MAX / dtype_size) return/throw an error to avoid overflow; only after validation and overflow check allocate newData and proceed to call withSharedPtr_impl_mut(newData, dtype). Ensure any error path returns a sensible failure instead of attempting the allocation.
🧹 Nitpick comments (2)
include/openPMD/Iteration.hpp (1)
290-299: Document the throwing behavior on the declaration.
Iteration.cppthrowserror::Internalwhen the cache is empty, but this is not mentioned in the Doxygen block here. Since the whole correctness ofSeries::indexOf()depends on callers understanding that the accessor throws (rather than returns0) for unowned iterations, it's worth surfacing in the header.📝 Proposed doc update
/** * `@brief` Get the cached iteration index. * This is the key under which this iteration is stored in the * Series::iterations map. Used internally for testing the index * caching optimization. * + * `@throws` error::Internal if the cached value is not available, i.e. + * if this Iteration was not inserted into a Series::iterations + * map via traits::GenerationPolicy<Iteration>. * `@return` The cached iteration index. */ uint64_t getCachedIterationIndex() const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/openPMD/Iteration.hpp` around lines 290 - 299, The Doxygen for getCachedIterationIndex() is missing that it throws error::Internal when the cache is empty; update the comment for uint64_t getCachedIterationIndex() const to document this throwing behavior explicitly (mention error::Internal is thrown on empty cache/unowned iteration) so callers like Series::indexOf() know it throws rather than returning 0; keep the rest of the description intact and reference the thrown exception in the `@throws` or brief sentence.include/openPMD/IO/ADIOS/ADIOS2IOHandler.hpp (1)
954-960: Prefer the flush wrapper here.This bypasses the new
AbstractIOHandler::flush()wrapper, including the “work queue was drained” invariant and flush-counter accounting. Since this is still inside theADIOS2IOHandlerdestructor body,this->flush(params)should dispatch toADIOS2IOHandler::flush_impl()while preserving wrapper behavior.♻️ Proposed change
- this->flush_impl(params); + this->flush(params);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/openPMD/IO/ADIOS/ADIOS2IOHandler.hpp` around lines 954 - 960, In the ADIOS2IOHandler destructor (~ADIOS2IOHandler) replace the direct call to this->flush_impl(params) with the wrapper call this->flush(params) using internal::defaultParsedFlushParams so the AbstractIOHandler::flush() wrapper (draining the work queue and updating flush counters) runs while still dispatching to ADIOS2IOHandler::flush_impl(); keep the try/catch and non‑throwing destructor behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/openPMD/IO/AbstractIOHandler.hpp`:
- Around line 268-275: The flush-skip logic currently treats equality as
"already flushed" causing newly enqueued operations to be skipped; in the
deferred-enqueue check in src/LoadStoreChunk.cpp (the code that captures
m_flushCounter and later compares the captured value to *m_flushCounter to
decide whether to call seriesFlush()), change the comparison from >= to > so
that only a strictly newer flush (m_flushCounter value increased) causes the
operation to be considered already run; update the comparison where the captured
counter (the local captured variable used by DeferredComputation/loadStoreChunk)
is compared to *m_flushCounter to use '>' instead of '>='.
In `@src/auxiliary/Memory.cpp`:
- Around line 255-267: The code currently uses OPENPMD_FOREACH_DATASET_DATATYPE
after Datatype.tpp has undef'd the macros and OPENPMD_INSTANTIATE only
instantiates mutable dtype pointers, causing missing const-type instantiations
and linker errors; to fix, re-include or restore the datatype macros before the
block that calls OPENPMD_FOREACH_DATASET_DATATYPE and expand OPENPMD_INSTANTIATE
to also instantiate std::shared_ptr<dtype const> (in addition to
std::shared_ptr<dtype>) so that WriteBuffer::WriteBuffer(std::shared_ptr<dtype
const>) and WriteBuffer::operator=(std::shared_ptr<dtype const> const&) are
emitted; update the instantiation section around the
OPENPMD_INSTANTIATE/OPENPMD_FOREACH_DATASET_DATATYPE usage and ensure the header
that defines the macros (the one removed by UndefDatatypeMacros.hpp) is included
again just prior to this instantiation block.
In `@src/IO/ADIOS/ADIOS2File.cpp`:
- Around line 93-97: The code allows mixing selected and unselected writes when
CanTheMemorySelectionBeReset is false, which lets a stale ADIOS2 variable
selection persist and corrupt later writes; update the storeChunk() logic in
ADIOS2File.cpp to, when CanTheMemorySelectionBeReset == false and
memorySelection.has_value() is false, explicitly reset the variable selection to
the full-buffer selection before writing (i.e., call the ADIOS2
Variable.SetSelection/full-range selection API on the same variable) or
alternatively reject the call with an error if mixing is detected — change the
branch that currently only checks memorySelection.has_value() to handle this
reset or validation and include a clear error path referencing
CanTheMemorySelectionBeReset, memorySelection, storeChunk(), and
warningMemorySelection so the behavior is deterministic on older ADIOS2
versions.
In `@src/IO/ADIOS/ADIOS2IOHandler.cpp`:
- Around line 1321-1325: The queryOnly branch currently sets
parameters.out->backendManagedBuffer = true and returns unconditionally; change
it to run the same eligibility checks as the real execution path before claiming
support. Specifically, when parameters.queryOnly is true perform the zero-extent
check (the extents-all-zero early-return), evaluate parameters.useSpan against
UseSpan::No, and for UseSpan::Auto ensure you reject if stream has operators or
BP5 constraints (the same operator/BP5 logic used later), and only then set
parameters.out->backendManagedBuffer = true (otherwise set false/return failure)
so the capability query mirrors actual execution behavior.
In `@src/LoadStoreChunk.cpp`:
- Around line 81-90: The lambda currently returns early when the current flush
counter is >= old_index, which skips seriesFlush() when counters are equal;
change the comparison in the early-exit condition inside the lambda that
captures old_index/current_index (from attr.IOHandler()->m_flushCounter) so it
only returns when !lock_current_index || *lock_current_index > old_index (i.e.
flip the >= to >) so that when *lock_current_index == old_index the code
proceeds to call attr.seriesFlush().
- Around line 112-124: When initializing m_extent from m_rc.getExtent() and
applying m_offset, validate each offset in m_offset against the corresponding
extent value before performing the subtraction in the loop inside the block
where m_extent is set; if any *it_o is greater than the corresponding *it_e,
reject the request (e.g., return an error/throw) instead of performing *it_e -=
*it_o to avoid underflow. Locate the loop that iterates with it_o/it_e in the
code that sets m_extent (the block using m_extent =
std::make_optional<Extent>(m_rc.getExtent()) and m_offset->begin()) and add a
guard that checks *it_o <= *it_e for each element, handling violations
explicitly (reject/raise) before doing the subtraction.
In `@src/RecordComponent.cpp`:
- Around line 35-40: Remove the duplicate include of "DatatypeMacros.hpp" in
RecordComponent.cpp: keep a single `#include` "openPMD/DatatypeMacros.hpp" and
delete the repeated include (or alternately add intervening `#undefs` if
intentional), so the macro definitions are only introduced once and no
redefinition diagnostics occur.
---
Outside diff comments:
In `@src/IO/HDF5/HDF5IOHandler.cpp`:
- Around line 1938-1991: The opaque conversion helper to_opaque() is disabling
the defer execution by setting do_run_this = false before wrapping; update
to_opaque() in auxiliary::defer (used by defer_close_memspace) so the returned
opaque defer retains the original "should run" flag and invokes the captured
cleanup (H5Sclose(memspace)) when destroyed — i.e., implement the opaque wrapper
to capture the original defer state and call the stored callable only if the
original do_run_this was true, rather than clearing it, so the memspace cleanup
in HDF5IOHandler (defer_close_memspace / H5Sclose) actually runs.
- Around line 1528-1658: The retry loop currently reassigns dataset_type (the
original H5Dget_type() handle) to next_type and uses defer_close_next_type which
closes next_type at end of the iteration, causing double-close and
use-after-close; fix by keeping the original H5Dget_type() handle separate
(e.g., keep original_type = dataset_type) and use a separate variable (e.g.,
probe_type) to walk H5Tget_super() results, collecting/closing only the
probe_type handles after the loop ends; update references to
defer_close_dataset_type to close original_type and remove/relocate
defer_close_next_type so intermediate probe types are not closed mid-loop but
cleaned up once the loop finishes or on error.
- Around line 1188-1205: The dataset_space and propertyList handles returned by
H5Dget_space() and H5Dget_create_plist() are leaked on every exit path; ensure
you call H5Sclose(dataset_space) and H5Pclose(propertyList) on all paths (normal
exit, after VERIFY failure, and when H5Pget_chunk() fails/throws). Wrap the
acquisition in a small RAII or try/finally-style cleanup around the block that
obtains dataset_space and propertyList (or explicitly close them before
throwing), and keep the existing VERIFY and the runtime_error throw but make
sure both handles are closed beforehand; reference the variables dataset_space,
propertyList and the calls H5Dget_space, H5Dget_create_plist, H5Pget_chunk,
H5Sclose, and H5Pclose when implementing the fix.
- Around line 2565-2626: The defer_close_dataType guard currently captures
dataType by reference and may end up closing member-owned handles
(m_H5T_LONG_DOUBLE_80_LE / m_H5T_CLONG_DOUBLE_80_LE) when dataType is reassigned
for the long-double fallback; fix by making the defer_close_dataType capture the
original caller-owned dataType by value and introduce a separate variable (e.g.,
readDataType) to hold the possibly-rewritten type used for reading; use dataType
for the close guard and use readDataType in the H5Dread call and in the H5Tequal
checks so member-owned handles are never closed by the local guard.
In `@src/RecordComponent.cpp`:
- Around line 787-793: The current check uses dse[i] < o[i] + e[i] which can
wrap on unsigned addition; change it to a subtractive check to avoid overflow by
testing if the offset exceeds the dataset or the extent exceeds the remaining
space: replace the condition with if (o[i] > dse[i] || e[i] > dse[i] - o[i]) and
throw the same runtime_error (adjust message if desired); apply the same fix to
the other occurrence that uses dse/o/e (the block around the later check
referenced in the comment).
---
Duplicate comments:
In `@src/LoadStoreChunk.cpp`:
- Around line 379-386: The explicit instantiations generated by
INSTANTIATE_METHOD_TEMPLATES and
INSTANTIATE_METHOD_TEMPLATES_WITH_AND_WITHOUT_EXTENT omit template argument
syntax so the compiler can't deduce dtype/type for ConfigureLoadStore::load and
ConfigureLoadStore::storeSpan; update the macros to emit explicit template
argument forms (e.g. use "template auto ConfigureLoadStore::load<dtype>() ->
..." and "template auto ConfigureLoadStore::storeSpan<type>() ->
DynamicMemoryView<type>;" and likewise for OPENPMD_ARRAY(type) variants) so the
template parameter is specified in the instantiation for each dtype/type.
In `@src/RecordComponent.cpp`:
- Around line 238-258: Before allocating the buffer in
RecordComponent::loadChunkAllocate_impl, validate the computed selection and
guard the multiplication numPoints * dtype_size for overflow: first call the
same validation that prepareLoadStore() uses (or invoke
prepareLoadStore().offset(...).extent(...).validate() or perform equivalent
checks on o and e) to reject invalid/huge selections, then compute size_t bytes
= numPoints; if (dtype_size != 0 && bytes > SIZE_MAX / dtype_size) return/throw
an error to avoid overflow; only after validation and overflow check allocate
newData and proceed to call withSharedPtr_impl_mut(newData, dtype). Ensure any
error path returns a sensible failure instead of attempting the allocation.
---
Nitpick comments:
In `@include/openPMD/IO/ADIOS/ADIOS2IOHandler.hpp`:
- Around line 954-960: In the ADIOS2IOHandler destructor (~ADIOS2IOHandler)
replace the direct call to this->flush_impl(params) with the wrapper call
this->flush(params) using internal::defaultParsedFlushParams so the
AbstractIOHandler::flush() wrapper (draining the work queue and updating flush
counters) runs while still dispatching to ADIOS2IOHandler::flush_impl(); keep
the try/catch and non‑throwing destructor behavior intact.
In `@include/openPMD/Iteration.hpp`:
- Around line 290-299: The Doxygen for getCachedIterationIndex() is missing that
it throws error::Internal when the cache is empty; update the comment for
uint64_t getCachedIterationIndex() const to document this throwing behavior
explicitly (mention error::Internal is thrown on empty cache/unowned iteration)
so callers like Series::indexOf() know it throws rather than returning 0; keep
the rest of the description intact and reference the thrown exception in the
`@throws` or brief sentence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 695c4b9d-aaae-4e08-bccf-ddb74625674f
📒 Files selected for processing (46)
CMakeLists.txtinclude/openPMD/Dataset.hppinclude/openPMD/Datatype.hppinclude/openPMD/Datatype.tppinclude/openPMD/IO/ADIOS/ADIOS2File.hppinclude/openPMD/IO/ADIOS/ADIOS2IOHandler.hppinclude/openPMD/IO/ADIOS/macros.hppinclude/openPMD/IO/AbstractIOHandler.hppinclude/openPMD/IO/DummyIOHandler.hppinclude/openPMD/IO/HDF5/HDF5IOHandler.hppinclude/openPMD/IO/HDF5/ParallelHDF5IOHandler.hppinclude/openPMD/IO/IOTask.hppinclude/openPMD/IO/InvalidatableFile.hppinclude/openPMD/IO/JSON/JSONIOHandler.hppinclude/openPMD/Iteration.hppinclude/openPMD/LoadStoreChunk.hppinclude/openPMD/LoadStoreChunk.tppinclude/openPMD/ParticleSpecies.hppinclude/openPMD/RecordComponent.hppinclude/openPMD/RecordComponent.tppinclude/openPMD/auxiliary/Defer.hppinclude/openPMD/auxiliary/Future.hppinclude/openPMD/auxiliary/Memory.hppinclude/openPMD/auxiliary/Memory_internal.hppinclude/openPMD/auxiliary/UniquePtr.hppinclude/openPMD/backend/Attributable.hppsrc/Datatype.cppsrc/IO/ADIOS/ADIOS2File.cppsrc/IO/ADIOS/ADIOS2IOHandler.cppsrc/IO/AbstractIOHandler.cppsrc/IO/AbstractIOHandlerImpl.cppsrc/IO/DummyIOHandler.cppsrc/IO/HDF5/HDF5IOHandler.cppsrc/IO/HDF5/ParallelHDF5IOHandler.cppsrc/IO/JSON/JSONIOHandler.cppsrc/IO/JSON/JSONIOHandlerImpl.cppsrc/LoadStoreChunk.cppsrc/RecordComponent.cppsrc/Series.cppsrc/auxiliary/Future.cppsrc/auxiliary/Memory.cppsrc/auxiliary/UniquePtr.cppsrc/binding/python/PatchRecordComponent.cpptest/CoreTest.cpptest/ParallelIOTest.cpptest/SerialIOTest.cpp
💤 Files with no reviewable changes (1)
- src/Datatype.cpp
✅ Files skipped from review due to trivial changes (1)
- src/IO/AbstractIOHandlerImpl.cpp
🚧 Files skipped from review as they are similar to previous changes (25)
- include/openPMD/Dataset.hpp
- src/IO/DummyIOHandler.cpp
- include/openPMD/IO/JSON/JSONIOHandler.hpp
- include/openPMD/auxiliary/UniquePtr.hpp
- src/IO/JSON/JSONIOHandler.cpp
- include/openPMD/IO/InvalidatableFile.hpp
- include/openPMD/IO/IOTask.hpp
- include/openPMD/IO/ADIOS/macros.hpp
- src/binding/python/PatchRecordComponent.cpp
- include/openPMD/IO/HDF5/ParallelHDF5IOHandler.hpp
- include/openPMD/IO/HDF5/HDF5IOHandler.hpp
- include/openPMD/ParticleSpecies.hpp
- src/IO/HDF5/ParallelHDF5IOHandler.cpp
- test/SerialIOTest.cpp
- include/openPMD/auxiliary/Defer.hpp
- src/IO/AbstractIOHandler.cpp
- include/openPMD/Datatype.hpp
- src/auxiliary/Future.cpp
- test/ParallelIOTest.cpp
- include/openPMD/backend/Attributable.hpp
- include/openPMD/auxiliary/Memory.hpp
- include/openPMD/auxiliary/Future.hpp
- src/Series.cpp
- include/openPMD/RecordComponent.tpp
- include/openPMD/LoadStoreChunk.tpp
Comment on lines
+268
to
+275
| /** Counter tracking the number of flush operations. This is later used to | ||
| * avoid repeated flushing in the DeferredComputation objects returned by | ||
| * the loadStoreChunk() API. (The counter is copied as a weak reference to | ||
| * the shared pointer, and the value is compared to the value upon enqueuing | ||
| * the operation. If the flush counter has proceeded past the old value, our | ||
| * operation has already been run.) */ | ||
| std::shared_ptr<unsigned long long> m_flushCounter = | ||
| std::make_shared<unsigned long long>(0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all flush-counter consumers and confirm equality is not treated as already flushed.
rg -n -C5 '\bm_flushCounter\b|\bold_index\b|\bseriesFlush\b|\bdeferFlush\b' --type=cpp --type=hppRepository: franzpoeschel/openPMD-api
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for flush counter and deferred flush logic without file type restrictions
rg -n 'm_flushCounter|old_index|seriesFlush|deferFlush' -C3 --max-count=100Repository: franzpoeschel/openPMD-api
Length of output: 31876
🏁 Script executed:
#!/bin/bash
# Check if LoadStoreChunk.cpp exists and examine the relevant lines
find . -name "LoadStoreChunk.cpp" -type fRepository: franzpoeschel/openPMD-api
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Examine AbstractIOHandler.hpp for flush counter definition
find . -name "AbstractIOHandler.hpp" -type fRepository: franzpoeschel/openPMD-api
Length of output: 111
Change the flush counter comparison from >= to > at src/LoadStoreChunk.cpp:86.
Newly enqueued operations capture the current flush counter value. The check currently uses >=, treating equality (no flush since enqueue) as "already flushed," which skips the intended seriesFlush() call. The comparison should be strict > to only skip when a flush has actually occurred since the operation was enqueued.
Required fix
- if (!lock_current_index || *lock_current_index >= old_index)
+ if (!lock_current_index || *lock_current_index > old_index)
{
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/openPMD/IO/AbstractIOHandler.hpp` around lines 268 - 275, The
flush-skip logic currently treats equality as "already flushed" causing newly
enqueued operations to be skipped; in the deferred-enqueue check in
src/LoadStoreChunk.cpp (the code that captures m_flushCounter and later compares
the captured value to *m_flushCounter to decide whether to call seriesFlush()),
change the comparison from >= to > so that only a strictly newer flush
(m_flushCounter value increased) causes the operation to be considered already
run; update the comparison where the captured counter (the local captured
variable used by DeferredComputation/loadStoreChunk) is compared to
*m_flushCounter to use '>' instead of '>='.
Comment on lines
+93
to
+97
| static constexpr char const *warningMemorySelection = | ||
| "[Warning] Using a version of ADIOS2 that cannot reset memory selections " | ||
| "on a variable, once specified. When using memory selections, then please " | ||
| "specify it explicitly on all storeChunk() calls. Further info: " | ||
| "https://github.com/ornladios/ADIOS2/pull/4169."; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ADIOS2, does Variable<T>::SetMemorySelection(Box<Dims>) persist on the Variable across subsequent Put calls until SetMemorySelection() is called with no arguments?
💡 Result:
Yes, in ADIOS2, Variable<T>::SetMemorySelection(Box<Dims>) sets a memory selection that persists on the Variable object across subsequent Put calls until explicitly changed, such as by calling SetMemorySelection with no arguments to reset it to the full memory extent (matching the variable's Count).
Citations:
- 1: Memory Selection for GPU pointers ornladios/ADIOS2#4337
- 2: Dynamic variable shaping not working as expected with MPI ornladios/ADIOS2#3709
- 3: Count and SelectionSize are not updated at new step for variable sized 1D global array ornladios/ADIOS2#3311
- 4: https://adios2.readthedocs.io/en/v2.10.2/advanced/gpu_aware.html
- 5: https://adios2.readthedocs.io/en/latest/advanced/gpu_aware.html
🏁 Script executed:
#!/bin/bash
# Description: Verify all memory-selection reset/fallback sites.
# Expectation: Older ADIOS2 paths should either overwrite with a default full-buffer memory selection or prevent unsafe later writes.
rg -n -C5 'CanTheMemorySelectionBeReset|SetMemorySelection|memorySelection' --type=cpp --type=hppRepository: franzpoeschel/openPMD-api
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for memory selection handling in ADIOS2File.cpp
cd src/IO/ADIOS
rg -n -C 5 'CanTheMemorySelectionBeReset|SetMemorySelection|memorySelection' ADIOS2File.cpp ADIOS2File.hpp 2>/dev/null || rg -n -C 5 'CanTheMemorySelectionBeReset|SetMemorySelection|memorySelection'Repository: franzpoeschel/openPMD-api
Length of output: 7722
🏁 Script executed:
#!/bin/bash
# Get the specific lines mentioned in the review
cat -n src/IO/ADIOS/ADIOS2File.cpp | sed -n '90,100p'Repository: franzpoeschel/openPMD-api
Length of output: 634
🏁 Script executed:
#!/bin/bash
# Check lines 127-139
cat -n src/IO/ADIOS/ADIOS2File.cpp | sed -n '125,145p'Repository: franzpoeschel/openPMD-api
Length of output: 1010
🏁 Script executed:
#!/bin/bash
# Check lines 212-223
cat -n src/IO/ADIOS/ADIOS2File.cpp | sed -n '210,230p'Repository: franzpoeschel/openPMD-api
Length of output: 846
🏁 Script executed:
#!/bin/bash
# Find the storeChunk method and how it handles memorySelection
rg -A 20 'void.*storeChunk\(' src/IO/ADIOS/ADIOS2File.cppRepository: franzpoeschel/openPMD-api
Length of output: 51
On older ADIOS2 versions, stale memory selections can corrupt subsequent writes without explicit memory selection.
When CanTheMemorySelectionBeReset is false, a write with memorySelection leaves that selection on the ADIOS2 variable. A later storeChunk() without memorySelection will skip the reset logic entirely (since memorySelection.has_value() is false) and reuse the stale selection, potentially corrupting the written data. The warning message acknowledges this by advising users to "specify it explicitly on all storeChunk() calls," but the code does not actively prevent unsafe mixing.
Either explicitly reset to full-buffer memory selection for writes without memorySelection on affected versions, or reject mixing selected and unselected writes for the same variable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/IO/ADIOS/ADIOS2File.cpp` around lines 93 - 97, The code allows mixing
selected and unselected writes when CanTheMemorySelectionBeReset is false, which
lets a stale ADIOS2 variable selection persist and corrupt later writes; update
the storeChunk() logic in ADIOS2File.cpp to, when CanTheMemorySelectionBeReset
== false and memorySelection.has_value() is false, explicitly reset the variable
selection to the full-buffer selection before writing (i.e., call the ADIOS2
Variable.SetSelection/full-range selection API on the same variable) or
alternatively reject the call with an error if mixing is detected — change the
branch that currently only checks memorySelection.has_value() to handle this
reset or validation and include a clear error path referencing
CanTheMemorySelectionBeReset, memorySelection, storeChunk(), and
warningMemorySelection so the behavior is deterministic on older ADIOS2
versions.
Comment on lines
+1321
to
+1325
| else if (parameters.queryOnly) | ||
| { | ||
| parameters.out->backendManagedBuffer = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how GET_BUFFER_VIEW queryOnly results are consumed.
# Expectation: Callers should not assume backendManagedBuffer=true unless the non-query path can also provide it.
rg -n -C4 '\bqueryOnly\b|GET_BUFFER_VIEW|backendManagedBuffer' --type=cpp --type=hppRepository: franzpoeschel/openPMD-api
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Fixed search without problematic --type flag
# Look for queryOnly, GET_BUFFER_VIEW, and backendManagedBuffer patterns
rg -n -C4 'queryOnly|GET_BUFFER_VIEW|backendManagedBuffer' --type=cppRepository: franzpoeschel/openPMD-api
Length of output: 13222
🏁 Script executed:
#!/bin/bash
# Examine the specific lines mentioned in the review
echo "=== Lines 1321-1325 ==="
sed -n '1315,1330p' src/IO/ADIOS/ADIOS2IOHandler.cpp
echo ""
echo "=== Lines 1343-1364 ==="
sed -n '1340,1370p' src/IO/ADIOS/ADIOS2IOHandler.cppRepository: franzpoeschel/openPMD-api
Length of output: 1574
🏁 Script executed:
#!/bin/bash
# Search for GetBufferView function definition and its full logic
rg -n 'GetBufferView|getBufferView' --type=cpp -A 50 | head -200Repository: franzpoeschel/openPMD-api
Length of output: 16461
Align queryOnly response with actual span eligibility.
When queryOnly=true, the function returns backendManagedBuffer=true without checking the UseSpan configuration, operator presence, BP5 version gates, or zero extent conditions that follow. This causes capability queries to report success when the actual operation would fail. The queryOnly path should apply the same eligibility checks as the real execution path before committing to backendManagedBuffer=true.
Control flow mismatch
- queryOnly path (lines 1321-1325): Returns
trueimmediately - Real path conditions:
- Zero extent check (lines 1330-1340): Returns
false - UseSpan::No (lines 1345-1347): Returns
false - UseSpan::Auto with operators/BP5 (lines 1349-1364): Returns
false
- Zero extent check (lines 1330-1340): Returns
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/IO/ADIOS/ADIOS2IOHandler.cpp` around lines 1321 - 1325, The queryOnly
branch currently sets parameters.out->backendManagedBuffer = true and returns
unconditionally; change it to run the same eligibility checks as the real
execution path before claiming support. Specifically, when parameters.queryOnly
is true perform the zero-extent check (the extents-all-zero early-return),
evaluate parameters.useSpan against UseSpan::No, and for UseSpan::Auto ensure
you reject if stream has operators or BP5 constraints (the same operator/BP5
logic used later), and only then set parameters.out->backendManagedBuffer = true
(otherwise set false/return failure) so the capability query mirrors actual
execution behavior.
Comment on lines
+81
to
+90
| auto index = attr.IOHandler()->m_flushCounter; | ||
| return [attr, | ||
| old_index = *index, | ||
| current_index = std::weak_ptr(index)]() mutable { | ||
| auto lock_current_index = current_index.lock(); | ||
| if (!lock_current_index || *lock_current_index >= old_index) | ||
| { | ||
| return; | ||
| } | ||
| attr.seriesFlush(); |
There was a problem hiding this comment.
Flip the flush-counter comparison.
In the normal case no flush has happened yet, so *lock_current_index == old_index; the current >= check returns early and skips the required seriesFlush(), so deferred loads can return before their queued read is executed.
Proposed fix
- if (!lock_current_index || *lock_current_index >= old_index)
+ if (!lock_current_index || *lock_current_index > old_index)
{
return;
}
attr.seriesFlush();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto index = attr.IOHandler()->m_flushCounter; | |
| return [attr, | |
| old_index = *index, | |
| current_index = std::weak_ptr(index)]() mutable { | |
| auto lock_current_index = current_index.lock(); | |
| if (!lock_current_index || *lock_current_index >= old_index) | |
| { | |
| return; | |
| } | |
| attr.seriesFlush(); | |
| auto index = attr.IOHandler()->m_flushCounter; | |
| return [attr, | |
| old_index = *index, | |
| current_index = std::weak_ptr(index)]() mutable { | |
| auto lock_current_index = current_index.lock(); | |
| if (!lock_current_index || *lock_current_index > old_index) | |
| { | |
| return; | |
| } | |
| attr.seriesFlush(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LoadStoreChunk.cpp` around lines 81 - 90, The lambda currently returns
early when the current flush counter is >= old_index, which skips seriesFlush()
when counters are equal; change the comparison in the early-exit condition
inside the lambda that captures old_index/current_index (from
attr.IOHandler()->m_flushCounter) so it only returns when !lock_current_index ||
*lock_current_index > old_index (i.e. flip the >= to >) so that when
*lock_current_index == old_index the code proceeds to call attr.seriesFlush().
Comment on lines
+112
to
+124
| if (!m_extent.has_value()) | ||
| { | ||
| m_extent = std::make_optional<Extent>(m_rc.getExtent()); | ||
| if (m_offset.has_value()) | ||
| { | ||
| auto it_o = m_offset->begin(); | ||
| auto end_o = m_offset->end(); | ||
| auto it_e = m_extent->begin(); | ||
| auto end_e = m_extent->end(); | ||
| for (; it_o != end_o && it_e != end_e; ++it_e, ++it_o) | ||
| { | ||
| *it_e -= *it_o; | ||
| } |
There was a problem hiding this comment.
Reject offsets beyond the dataset before subtracting.
When extent is defaulted, *it_e -= *it_o underflows if the requested offset is larger than the dataset extent, producing a huge selection that can later drive oversized allocation or invalid I/O.
Proposed guard
auto end_e = m_extent->end();
for (; it_o != end_o && it_e != end_e; ++it_e, ++it_o)
{
+ if (*it_o > *it_e)
+ {
+ throw std::runtime_error(
+ "Chunk offset does not reside inside dataset.");
+ }
*it_e -= *it_o;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!m_extent.has_value()) | |
| { | |
| m_extent = std::make_optional<Extent>(m_rc.getExtent()); | |
| if (m_offset.has_value()) | |
| { | |
| auto it_o = m_offset->begin(); | |
| auto end_o = m_offset->end(); | |
| auto it_e = m_extent->begin(); | |
| auto end_e = m_extent->end(); | |
| for (; it_o != end_o && it_e != end_e; ++it_e, ++it_o) | |
| { | |
| *it_e -= *it_o; | |
| } | |
| if (!m_extent.has_value()) | |
| { | |
| m_extent = std::make_optional<Extent>(m_rc.getExtent()); | |
| if (m_offset.has_value()) | |
| { | |
| auto it_o = m_offset->begin(); | |
| auto end_o = m_offset->end(); | |
| auto it_e = m_extent->begin(); | |
| auto end_e = m_extent->end(); | |
| for (; it_o != end_o && it_e != end_e; ++it_e, ++it_o) | |
| { | |
| if (*it_o > *it_e) | |
| { | |
| throw std::runtime_error( | |
| "Chunk offset does not reside inside dataset."); | |
| } | |
| *it_e -= *it_o; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LoadStoreChunk.cpp` around lines 112 - 124, When initializing m_extent
from m_rc.getExtent() and applying m_offset, validate each offset in m_offset
against the corresponding extent value before performing the subtraction in the
loop inside the block where m_extent is set; if any *it_o is greater than the
corresponding *it_e, reject the request (e.g., return an error/throw) instead of
performing *it_e -= *it_o to avoid underflow. Locate the loop that iterates with
it_o/it_e in the code that sets m_extent (the block using m_extent =
std::make_optional<Extent>(m_rc.getExtent()) and m_offset->begin()) and add a
guard that checks *it_o <= *it_e for each element, handling violations
explicitly (reject/raise) before doing the subtraction.
Comment on lines
35
to
+40
| // comment so clang-format does not move this | ||
| #include "openPMD/DatatypeMacros.hpp" | ||
|
|
||
| // comment | ||
| #include "openPMD/DatatypeMacros.hpp" | ||
|
|
There was a problem hiding this comment.
Remove the duplicate DatatypeMacros.hpp include.
The second include redefines the same macros without an intervening undef; this can trip macro-redefinition diagnostics under stricter warning settings.
Proposed cleanup
// comment so clang-format does not move this
`#include` "openPMD/DatatypeMacros.hpp"
-
-// comment
-#include "openPMD/DatatypeMacros.hpp"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // comment so clang-format does not move this | |
| #include "openPMD/DatatypeMacros.hpp" | |
| // comment | |
| #include "openPMD/DatatypeMacros.hpp" | |
| // comment so clang-format does not move this | |
| `#include` "openPMD/DatatypeMacros.hpp" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/RecordComponent.cpp` around lines 35 - 40, Remove the duplicate include
of "DatatypeMacros.hpp" in RecordComponent.cpp: keep a single `#include`
"openPMD/DatatypeMacros.hpp" and delete the repeated include (or alternately add
intervening `#undefs` if intentional), so the macro definitions are only
introduced once and no redefinition diagnostics occur.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/auxiliary/Future.cpp (1)
8-8: Remove the placeholder comment.Line 8 looks like leftover scaffolding and can be dropped.
Proposed cleanup
-// comment -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auxiliary/Future.cpp` at line 8, Remove the leftover placeholder comment in Future.cpp: locate the stray comment near the top of the file (the scaffolding placeholder) and delete it so the file contains only meaningful code/comments; ensure no other changes are made to surrounding declarations or includes in Future.cpp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auxiliary/Future.cpp`:
- Around line 87-88: The move-assignment operator
DeferredComputation<T>::operator=(DeferredComputation &&) noexcept currently
defaulted can destroy an existing OneTimeTask alternative without running
DeferredComputation's destructor logic, silently dropping pending tasks (e.g.,
deferFlush ownership). Change the move-assignment to explicitly drain/execute
the lhs pending task before replacing it: call the same "execute pending task
and log failures" routine used by DeferredComputation's destructor (or extract
that logic into a shared helper) so lhs's pending OneTimeTask is run and any
errors are logged, then perform the move of rhs into lhs and leave rhs in a
moved-from state; ensure the helper is invoked from both the destructor and
operator= to avoid duplication.
---
Nitpick comments:
In `@src/auxiliary/Future.cpp`:
- Line 8: Remove the leftover placeholder comment in Future.cpp: locate the
stray comment near the top of the file (the scaffolding placeholder) and delete
it so the file contains only meaningful code/comments; ensure no other changes
are made to surrounding declarations or includes in Future.cpp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e04585a-3b10-4289-bcdf-ab8d8957048d
📒 Files selected for processing (2)
include/openPMD/auxiliary/Future.hppsrc/auxiliary/Future.cpp
✅ Files skipped from review due to trivial changes (1)
- include/openPMD/auxiliary/Future.hpp
Comment on lines
+87
to
+88
| auto DeferredComputation<T>::operator=(DeferredComputation &&) noexcept( | ||
| noexcept_move) -> DeferredComputation & = default; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect DeferredComputation move-assignment behavior and related tests.
printf 'DeferredComputation move assignment implementation:\n'
rg -n -C5 'DeferredComputation<T>::operator=\(DeferredComputation &&\)|operator=\(DeferredComputation &&\).*default' src include
printf '\nOneTimeTask move assignment implementation:\n'
rg -n -C5 'OneTimeTask<T>::operator=\(OneTimeTask &&other\)' src include
printf '\nTests mentioning DeferredComputation / OneTimeTask move behavior:\n'
rg -n -C4 --glob '*test*' --glob '*Test*' 'DeferredComputation|OneTimeTask|move assignment|operator=' .Repository: franzpoeschel/openPMD-api
Length of output: 2645
🏁 Script executed:
# Examine the full DeferredComputation structure and destructor
sed -n '1,120p' src/auxiliary/Future.cppRepository: franzpoeschel/openPMD-api
Length of output: 3050
🏁 Script executed:
# Check the context from LoadStoreChunk.cpp
sed -n '230,250p' src/LoadStoreChunk.cppRepository: franzpoeschel/openPMD-api
Length of output: 758
🏁 Script executed:
# Search for the definition of DeferredComputation to understand its members
rg -n -B2 -A10 'class DeferredComputation|struct DeferredComputation' include srcRepository: franzpoeschel/openPMD-api
Length of output: 2379
🏁 Script executed:
# Check if OneTimeTask is used directly outside DeferredComputation
rg -n 'OneTimeTask<' src include --type cpp | grep -v 'DeferredComputation\|template\|variant' | head -20Repository: franzpoeschel/openPMD-api
Length of output: 1243
🏁 Script executed:
# Verify that OneTimeTask has no user-defined destructor
rg -n '~OneTimeTask' src includeRepository: franzpoeschel/openPMD-api
Length of output: 51
🏁 Script executed:
# Check if there are any tests for DeferredComputation move operations specifically
rg -n -C3 'move.*DeferredComputation|DeferredComputation.*move|operator=.*DeferredComputation' test --type cppRepository: franzpoeschel/openPMD-api
Length of output: 51
🏁 Script executed:
# Search for any existing move assignment test patterns
rg -n 'DeferredComputation.*=' test --type cpp | head -10Repository: franzpoeschel/openPMD-api
Length of output: 51
Drain the existing pending task before move-assigning.
Defaulted move assignment allows std::variant's assignment operator to destroy the old OneTimeTask alternative without invoking DeferredComputation's destructor, which contains the execution logic for pending tasks. Since OneTimeTask has only a trivial destructor, any valid pending task is silently discarded. In load scenarios, that task can own deferFlush(m_rc) (see src/LoadStoreChunk.cpp:237-240), so lhs = std::move(rhs) may silently skip the pending flush for lhs.
Proposed fix direction
template <typename T>
-auto DeferredComputation<T>::operator=(DeferredComputation &&) noexcept(
- noexcept_move) -> DeferredComputation & = default;
+auto DeferredComputation<T>::operator=(DeferredComputation &&other) noexcept(
+ noexcept_move) -> DeferredComputation &
+{
+ if (this != &other)
+ {
+ try
+ {
+ std::visit(
+ auxiliary::overloaded{
+ [](detail::OneTimeTask<T> &task) {
+ if (task.members.m_task_valid)
+ {
+ std::move(task)();
+ }
+ },
+ [](detail::CachedValue<T> &) {}},
+ this->m_task);
+ }
+ catch (std::exception const &e)
+ {
+ std::cerr << "[DeferredComputation] Error in move assignment: '"
+ << e.what() << "'." << std::endl;
+ }
+ catch (...)
+ {
+ std::cerr
+ << "[DeferredComputation] Unknown error in move assignment."
+ << std::endl;
+ }
+
+ this->m_task = std::move(other.m_task);
+ }
+ return *this;
+}Consider extracting the shared "execute pending task and log failures" logic from the destructor to avoid duplicating it here.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto DeferredComputation<T>::operator=(DeferredComputation &&) noexcept( | |
| noexcept_move) -> DeferredComputation & = default; | |
| auto DeferredComputation<T>::operator=(DeferredComputation &&other) noexcept( | |
| noexcept_move) -> DeferredComputation & | |
| { | |
| if (this != &other) | |
| { | |
| try | |
| { | |
| std::visit( | |
| auxiliary::overloaded{ | |
| [](detail::OneTimeTask<T> &task) { | |
| if (task.members.m_task_valid) | |
| { | |
| std::move(task)(); | |
| } | |
| }, | |
| [](detail::CachedValue<T> &) {}}, | |
| this->m_task); | |
| } | |
| catch (std::exception const &e) | |
| { | |
| std::cerr << "[DeferredComputation] Error in move assignment: '" | |
| << e.what() << "'." << std::endl; | |
| } | |
| catch (...) | |
| { | |
| std::cerr | |
| << "[DeferredComputation] Unknown error in move assignment." | |
| << std::endl; | |
| } | |
| this->m_task = std::move(other.m_task); | |
| } | |
| return *this; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/auxiliary/Future.cpp` around lines 87 - 88, The move-assignment operator
DeferredComputation<T>::operator=(DeferredComputation &&) noexcept currently
defaulted can destroy an existing OneTimeTask alternative without running
DeferredComputation's destructor logic, silently dropping pending tasks (e.g.,
deferFlush ownership). Change the move-assignment to explicitly drain/execute
the lhs pending task before replacing it: call the same "execute pending task
and log failures" routine used by DeferredComputation's destructor (or extract
that logic into a shared helper) so lhs's pending OneTimeTask is run and any
errors are logged, then perform the move of rhs into lhs and leave rhs in a
moved-from state; ensure the helper is invoked from both the destructor and
operator= to avoid duplication.
84c9cd7 to
da11934
Compare
test/Files_Core/read_nonexistent_attribute.cpp:23:10: fatal error: catch2/catch_to
string.hpp: No such file or directory
23 | #include <catch2/catch_tostring.hpp>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
bit of code duplication, but better than CRT complexity
for more information, see https://pre-commit.ci
for when no computation is needed
for more information, see https://pre-commit.ci
franzpoeschel
force-pushed
the
adios2-memory-selection
branch
from
7182268 to
3ba15d5
Compare
| #endif | ||
| } | ||
|
|
||
| TEST_CASE("future_test", "[auxiliary]") |
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
3 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests