ESCOMP / ESCOMP/CTSM

Improve design and error checking in decompMod and decompInitMod

Open
#3,448 0 comments 0 reactions 1 assignee Claimed by @ekluzek View on GitHub
b4b code health enhancement testing
Dominant language
Fortran
Stars
352
Forks
361
Avg merge
2d 21h
Merged PRs (30d)
7

Description

It turns out there are some design issues and lack of error checking decompMod, and decompInitMod that lead to problems like in #3447. This is where a segmentation fault happens in unrelated places in the code that aren't obvious to track down, because of this. This is an example of brittle code that's hard to work with, that also doesn't have sufficient testing around it, so is harder to work with and takes more time to do so.

Design issues I'm seeing now:
- [x] decompInitMod allocates and sets module data from decompMod, rather than having decompMod do this.
- [x] decompMod doesn't have clean allocate/deallocate methods
- [ ] decompMod has many functions that don't do error checking that the structures are even setup
- [ ] decompMod should initialize values before they are setup such that errors will be triggered if used before set
- [ ] decompMod doesn't do error checking for valid values from the decomp setup
- [ ] The subroutines in decompInit are too long, without clear purposes
- [x] get_proc_bounds is initially used to only return begg/endg, and later for everything, this usage isn't clear from the code...
- [x] The code in decompInit is ordered, without it being easy/ovious to change the order things are done
- [ ] Code is duplicated between get_proc_bounds and get_proc_clumps (maybe OK now, but with additional error checking, would be good to have the duplicated code in a private subroutine shared by both)
- [x] Pointers aren't set to null() initially so that you can check they are valid before using them

Error checking:
- [x] Error checking of namelist items should be done as close to the top as possible
- [x] Error checking that nsegspc is valid should be done as it can be set by the user
- [ ] Should check that the decomp has been setup, before you can use the functions in decompMod
- [ ] Appropriate range checking of inputs, and the internal arrays and dimension sizes should be done in the decompMod functions

Testing:
- [x] Adding self tests would be useful to ensure changes are correct and stay correct with development
- [x] Adding a serial unit tester for the most basic things and error checking testing would be helpful
- [x] Should add a standard case test to run to the decomp_init test, to make sure a normal test without the special testing framework still works

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.