Skip to content

Fix docstrings, remove dead typedef, and fix getElapsedTimeStep off-by-one - #1412

Open
dweep-desai wants to merge 3 commits into
FLAMEGPU:masterfrom
dweep-desai:fix/code-quality-dead-code-docstrings-obo
Open

dweep-desai wants to merge 3 commits into
FLAMEGPU:masterfrom
dweep-desai:fix/code-quality-dead-code-docstrings-obo

Conversation

@dweep-desai

@dweep-desai dweep-desai commented Sep 1, 2026

Copy link
Copy Markdown

Summary

Code quality PR addressing 3 open issues: incorrect/copy-pasted docstrings, an unused typedef, and a newly discovered off-by-one bounds check bug in getElapsedTimeStep().

Each fix is in a distinct commit for ease of review.

Changes

Commit 1: Fix getElapsedTime* docstrings (Fixes #1392)

The docstring for CUDASimulation::getElapsedTimeExitFunctions incorrectly referred to stepFunctions() instead of exitFunctions(). As suggested in the issue, a wider pass was done over all getElapsedTime* docstrings, revealing:

  • getElapsedTimeRTCInitialisation: Typo "iniitliased" corrected to "initialised".
  • All four methods: Copy-pasted @return elapsed time of last simulation call in seconds. corrected to method-specific descriptions.
  • getElapsedTimeSteps comment: Typos "mutabililty" and "performacne" corrected.

Commit 2: Remove unused CUDARTCFuncMapPair typedef (Fixes #1378)

CUDAAgent::CUDARTCFuncMapPair was defined as a typedef but never referenced anywhere in the codebase. The docstring described it as "Element type of CUDARTCFuncMap" but CUDARTCFuncMap is a std::map, not using this pair type. Removed the typedef and its docstring.

Commit 3: Fix getElapsedTimeStep off-by-one bounds check (Fixes #1411)

CUDASimulation::getElapsedTimeStep(unsigned int step) used step > size() instead of step >= size(). When step == size() (e.g., requesting step 10 after a 10-step simulation where valid indices are 0–9):

  • The guard condition 10 > 10 evaluated to false, passing the check
  • .at(10) then threw std::out_of_range instead of exception::OutOfBoundsException

Also enables the previously commented-out // @todo out-of-bounds test and adds a boundary regression test.

Files Changed

File Change
include/flamegpu/simulation/CUDASimulation.h Fix docstrings for all getElapsedTime* methods
include/flamegpu/simulation/detail/CUDAAgent.h Remove unused CUDARTCFuncMapPair typedef
src/flamegpu/simulation/CUDASimulation.cu Fix off-by-one bounds check and comment typos
tests/test_cases/simulation/test_cuda_simulation.cu Enable + extend out-of-bounds regression tests

Validation

  • cpplint passed on all 4 modified files with 0 errors.
  • All changes are backwards-compatible; no API changes.

Fixes #1392, #1378, #1411

@dweep-desai

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@Robadob

Robadob commented Sep 7, 2026

Copy link
Copy Markdown
Member
  1. Sure
  2. Yeah, I've removed many others during earlier refactors ,this one must have been missed.
  3. Redundant, a fix for UNIFIED_GPU_MEMORY unfinished code? #1396 is already pending, in AMD GPU Support via HIP/ROCm #1379
  4. Good catch.

Presumably you're using static analysis/AI?

Ideally each fix should be in a distinct commit, this makes it easier to review them properly.

The team is currently busy at a national conference, we'll hopefully be able to discuss this internally in a week or two.

- getElapsedTimeExitFunctions: stepFunctions() -> exitFunctions()
- getElapsedTimeRTCInitialisation: fix typo iniitliased -> initialised
- All getElapsedTime* methods: fix copy-pasted @return tags to be method-specific
- getElapsedTimeSteps comment: fix typos mutabililty -> mutability, performacne -> performance

Fixes FLAMEGPU#1392
The typedef was defined as the element type of CUDARTCFuncMap but was never
referenced anywhere in the codebase. Other similar dead typedefs were removed
in earlier refactors; this one was missed.

Fixes FLAMEGPU#1378
The bounds check used > instead of >=, allowing step == size() to pass the
guard. std::vector::at() then threw std::out_of_range instead of the
project's own exception::OutOfBoundsException.

Also enables the previously commented-out @todo out-of-bounds test and adds
a boundary regression test for step == STEPS.

Fixes FLAMEGPU#1411
@dweep-desai
dweep-desai force-pushed the fix/code-quality-dead-code-docstrings-obo branch from 6897e3b to a45b827 Compare September 7, 2026 14:50
@dweep-desai

Copy link
Copy Markdown
Author

Thanks for the review @Robadob!

I've addressed your feedback:

  1. Dropped the UNIFIED_GPU_MEMORY removal (UNIFIED_GPU_MEMORY unfinished code? #1396) — I see it's already handled in PR AMD GPU Support via HIP/ROCm #1379. Apologies for the overlap.
  2. Split into 3 distinct commits, one per fix:

Force-pushed to update the branch. No rush on the review — enjoy the conference!

@dweep-desai dweep-desai changed the title Fix docstrings, remove dead code, and fix getElapsedTimeStep off-by-one Fix docstrings, remove dead typedef, and fix getElapsedTimeStep off-by-one Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants