bbakernoaa / bbakernoaa/AMIO

Consider adopting `[[nodiscard]]` and `[[maybe_unused]]` consistently across AMIO

Open
#12 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.