Consider adopting `[[nodiscard]]` and `[[maybe_unused]]` consistently across AMIO
- Dominant language
- C++
- Stars
- 1
- Forks
- 3
- Avg merge
- 2d 30m
- Merged PRs (30d)
- 5
Description
Follow-up from review of #9. Neither `[[nodiscard]]` nor `[[maybe_unused]]` currently appears anywhere in AMIO. Rather than mark only the functions touched by that PR — which would leave new code stricter than the surrounding code — this issue tracks adopting both uniformly.
## 1. `[[nodiscard]]` on status-returning C++ functions
18 C++ functions return `amio_status_t`; none are marked.
- `src/c_boundary/amio_core.hpp` — 15 free functions in `amio::detail` (`init`, `finalize`, `open_dataset`, `close_dataset`, `write`, `read`, `get_var_attribute_text`, `get_var_attribute_double`, `flush`, `close`, `wait`, `release_view`, `view_data`, `view_shape`, `view_dtype`)
- `src/c_boundary/handle_table.hpp` — `HandleTable::lookup`, `HandleTable::release`
- `src/prefetch/prefetch_queue.hpp` — `PrefetchQueue::get_buffer`
The public C API in `include/amio/amio.h` is out of scope: it is C99, `[[nodiscard]]` is C23, and `tests/header_isolation` compiles that header with `-pedantic -Werror` under C99.
### Impacts to resolve
**Placement.** The attribute must go on the declaration in the header. Putting it on the definition in `amio_core.cpp` has no effect, because every caller sees only the header declaration.
**`HandleTable::release` — return value discarded in production code.** Three sites in `src/c_boundary/amio_core.cpp`: in `finalize` (dataset teardown loop, and the core handle release) and in `close_dataset` (step 5). Marking `release` requires first deciding whether ignoring a failed release is intentional best-effort teardown or a latent bug. This is the one genuinely open design question here.
**Return value discarded in tests.** Mechanical, but touches files unrelated to whatever PR does the work:
- `amio::detail::release_view` — the `release()` fixture helper in `tests/unit/test_read_bbox_validation.cpp`, `test_read_capacity_guard.cpp`, `test_read_close_guard.cpp`
- `PrefetchQueue::get_buffer` — two calls in `tests/unit/test_read_prefetch.cpp` (the `schedule_next` bounds test)
For the test sites, prefer asserting `== AMIO_OK` over silencing with a cast — a failing release in a test is real signal.
## 2. `(void)param;` → `[[maybe_unused]]`
47 occurrences of the C-style discard idiom across 9 files; PR #9 converted the 4 it introduced, leaving 43:
- `src/drivers/grib2/grib2_driver.cpp` (9)
- `src/drivers/netcdf/netcdf_driver.cpp` (7)
- `src/drivers/zarr/zarr_driver.cpp` (9)
- `src/factory/backend_driver.hpp` (3)
- `src/c_boundary/amio_core.cpp` (1)
- `src/config/config_loader.cpp` (1)
- `src/drivers/zarr/zarr_nczarr_fallback.cpp` (1)
- `src/workers/comm_split.cpp` (1)
- `src/workers/worker_pool.cpp` (1)
The project is C++17-capable (`CMAKE_CXX_STANDARD 20`), so `[[maybe_unused]]` on the parameter declaration is the idiomatic form and puts the intent in the signature. Note that several sites carry an explanatory trailing comment (e.g. `(void)format; // Reserved for future use; CONF auto-detects.`) that should be preserved.
Purely mechanical and low-risk — reasonable to split into its own PR from part 1.
## Suggested sequencing
1. `[[maybe_unused]]` sweep (mechanical, no behavior change)
2. `[[nodiscard]]` on `amio::detail` + `PrefetchQueue::get_buffer`, with the affected test call sites converted to assertions
3. `HandleTable::release` / `lookup`, once the production-discard question is settled
cc: @DWesl (raised in #9)
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the listed declarations in src/c_boundary/amio_core.hpp, src/c_boundary/handle_table.hpp, and src/prefetch/prefetch_queue.hpp, then review the discard sites and tests named in the issue. Apply the [[maybe_unused]] sweep first, and run the affected unit tests. Done means the attributes are adopted consistently, discarded test returns are asserted, and the HandleTable::release teardown behavior has a documented decision.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend, testing-qa
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100