Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release/devel v7.2.0 #372

Merged
merged 93 commits into from
Oct 24, 2023
Merged

Release/devel v7.2.0 #372

merged 93 commits into from
Oct 24, 2023

Conversation

dschlaep
Copy link
Member

No description provided.

Nicholas Persley added 30 commits August 25, 2023 19:54
- Removed instances of using `snprintf()` previous to `LogError()` instead of injecting the message directly into `LogError()`
	* These instances have been updated to use `LogError() directly`
	* `SW_MDL_read()` still does something similar to this, due to the necessity of adding to the error message before reporting
- Outside of echoing, (e.g., `_echo_inputs()`), there exists no use of LOGNOTE, instead, everything uses LOGWARN, LOGFATAL, etc.
- This explicitly allows notes to only be used for the echoing of in/outputs when requested
- To prepare for future error logging and to make mallocing as consistent as possible throughout the program, most standing uses of `malloc()` have been swapped with `Mem_Malloc()`
- Updated function interfaces to include LOG_INFO if they did not have it previously to send into `Mem_Malloc()`
* Tests update the calls to called functions

Special Cases
	- Tests
		* Tests will use `malloc()` instead of `Mem_Malloc()` due to an unnecessary need for error checking and provides superfluous actions to execute
	- fCreateBlockInfo
		* Cannot call `Mem_Malloc()` due to it being called in the function, so it will remain to use `malloc()`
	- sw_strdup
		* This function needs more deliberation before `malloc()` can be changed to `Mem_Malloc()`
- Previously, the function, `Str_CompareI()` would allocate memory to compare two different strings
- Because this function needs LOG_INFO for Mem_Malloc, we cannot include `SW_datastructs.h` because `SW_datastructs.h` depends on generic.c, causing problems
- Rewrote to no longer use `malloc()` or `Mem_Malloc()` and is custom enough to no longer use C-provided comparison functions and only use `tolower()`
- Instances of `sw_error()` no longer exist outside of `LogError()`
- If a message is meant to be an error, the call to `LogError()` uses LOGFATAL, otherwise, it uses LOGEXIT

- `sw_init_args()` no longer uses `sw_error()` to make the error handling consistent throughout the program

- `sw_check_log()` outputs it's message through `fprintf()` (stderr) instead of `sw_error()`
- No longer use `LogError()` when outputting echo information (e.g., `_echo_inputs()`) - instead report directly through the command line
- In future development, the number of messages will be restricted, and losing the reporting through `LogError()` reduces memory consumption and complexity of implementation
- Some functions lose LOG_INFO as an argument
	* `_echo_outputs()` still "uses" this because the mock output function(s) is in need of LOG_INFO
- For future plans of error handling, we need to make sure all possible dynamically allocated values are initialized to NULL
	* This allows the checking for NULL when deallocating values that may not have been set yet (i.e., contains junk values)

SW_CTL_init_ptrs
- Once the variables in main() have been created, we immediately set pointers to NULL
- Calls helper functions in the different parts of the code (e.g., weather, vegetation establishment, etc.)

SW_*_init_ptrs()
- Initializes respective pointers found within `SW_*_deconstruct()` to NULL or a value that should be initialized (e.g., SW_VegEstab.count)

Tests
- Added call to `SW_CTL_init_ptrs()` in `setup_AllTest_for_tests()`
LOG_INFO
- Gains variables to store and keep track of multiple warning messages and one error message for reporting later in the program

SW_Defines.h
- Gains a new define - MAX_MSGS - to signify the maximum number of messages a message array in LOG_INFO can hold
SW_Output_Mock.c
- Removed the usage of extra functions in `_echo_outputs()` and replaced constants with NULL or enum values that were previously a variable
	* The reason this was not all the way abolished was that "test_severe" argues that sumof_*, averagefor, and collect_sums are not used

_echo_*
- Lose LOG_INFO as a parameter, if they previously had one
- `_echo_inputs()` gains a local instance of LOG_INFO
sw_init_logs
- Using a given instead of LOG_INFO, this function initializes all values within LOG_INFO to default values

sw_write_logs
- Using an instance of LOG_INFO that was used throughout the program, this function writes out all warnings and error messages (if any)
- Closes the log file and notifies the user of something being logged, if applicable
- Previously, "errorMsg" and "warningMsgs" in LOG_INFO were pointers or an array of pointers
- In the future, we cannot use `Mem_Malloc()` or `Str_Dup()` in `LogError()`, so we must only copy the strings rather than allocate and copy
- SW_Defines.h gains a new define - MAX_LOG_MSG which specifies the biggest possible message to hold
- `sw_init_logs()` no longer initializes warningMsgs
- `LogError()` no longer reports messages to the logfile but now stores error/warning messages in LOG_INFO
- These messages will be output by `sw_write_logs()`
- If STEPWAT is defined, and an error or exit key is received, STEPWAT2 will still crash

Tests
- Because the code no longer throws an error and crashes the program, unit tests can no longer test for this
- Removed the calls to `EXPECT_DEATH_IF_SUPPORTED()` and use GoogleMock's `EXPECT_THAT()` to test for error messages
- Renamed the function `silent_tests()` to `init_silent_tests()` which now initialized LOG_INFO as well as sets the log-file pointer to NULL
- Instead of wasting time checking if a current time period is to be set, we now initialize aggregated output to NULL
- It was found that the macro/function `strdup()`/`sw_strdup()` was only used in one place in SOILWAT2
- Replaced this one instance with `Str_Dup()`
- Removed define(s) for "strdup"
- Removed the function `sw_strdup()`

- Removed mention of `strdup()` in the makefile (within an example)
- Because the only possible use of `sw_error()` is now in `filefuncs.c`, the function no longer needs to be exported
- Moved function prototype to the beginning of `filefuncs.c`
- This function prototype cannot be static due to the flag [-Wunused-function]
- Remove use of `sw_error()`in tests
- `sw_init_logs()` gains a new parameter for the initialization of the log file pointer
- Replaced use of `init_silent_tests()` with `sw_init_logs()` directly
- Removed LOGFATAL, LOGEXIT, LOGQUIET, and LOGNOTE - these are no longer used or were excess, making message logging more confusing
- Constrain `LogError()` messages to two types LOGWARN and LOGERROR
- Update all instances of LOGEXIT and LOGFATAL to LOGERROR
- Only define `sw_error()` when STEPWAT2 is in use
- `sw_error()` is to be possibly used when STEPWAT2 is being used
- Currently, the function is doing excessive things just for a possible instance in STEPWAT2
- Replaced the call to this function in `LogError()` with `exit()` and a print out of the error
- Swapped the use of `vsnprintf()` with `vsprintf()` in `LogError()`
- The "too many warnings" message in `sw_write_logs()` now includes the number of found warnings and the number of messages printed
- Because tests no longer use a macro like `EXPECT_DEATH_IF_SUPPORTED()`, the convention of naming the test suite "...DeathTest" is no longer needed
- Actual test names remain to have "*DeathTest" to specify they're supposed to throw an error
- "sw_testhelpers.h" loses class that was used for death tests (does not inherit from "::testing::Test")
- Removed `setup_AllTest_for_tests()`
dschlaep and others added 15 commits October 10, 2023 10:39
Tests need now to deal with errors after the SOILWAT2 code doesn't immediately crash on error anymore

-> call `sw_fail_on_error()` after each function call that uses "LogInfo" as argument (unless that call is to test an error situation)
-> that is crash the tests on unexpected errors
- see commit 4e07625 "Merge pull request #371 from DrylandEcology/feature_remove_debug_mem"

see #369
- new type "sw_random_t"
** for rSOILWAT2 defined as "int" (but unused)
** for SOILWAT2 and STEPWAT2 defined as "pcg32_random_t" (as previously)

-> "pcg_basic.h" header now only included by "SW_Defines.h" (for typedef) and by "rands.c" (function calls) -- and only if not rSOILWAT2
-> makefile has new target "libr" to build library without pcg (for rSOILWAT2)

- addressing issue #373 "rSOILWAT2 does not use PCG for random numbers - remove requirement on its presence"
Encapsulate RNG variable in dedicated type

- new type "sw_random_t"
** for rSOILWAT2 defined as "int" (but unused)
** for SOILWAT2 and STEPWAT2 defined as "pcg32_random_t" (as previously)

- "pcg_basic.h" header now only included by "SW_Defines.h" (for typedef) and by "rands.c" (function calls) -- and only if not rSOILWAT2 -> makefile has new target "libr" to build library without pcg (for rSOILWAT2)

- close #373 "rSOILWAT2 does not use PCG for random numbers - remove requirement on its presence"
Mem_Realloc()
- Free passed in pointer if reallocation fails

getfiles()
- Free duplicated string before the first possible return
- No longer initialize "flist" elements when "created" - when allocation fails the pointer returned is NULL, so it would try to set an element within something that doesn't exist
- Update calls to freeGetfilesEarly to include zero-found elements (nfound)

freeGetfilesEarly()
- Make sure "flist" is not NULL before trying to loop through and free, instead of testing individual elements
- All history is now a triple pointer to update outside of the function
- Assists in rSOILWAT2's ability to call the function without SW_WEATHER
- Adjust function to dereference "allHist"
- note: issue #375 "gcc13 doesn't compile test program"
…WEATHER

- see also commit c439bc9 "`allocateAllWeather()` uses all-weather history, not SW_WEATHER"

- now, `allocateAllWeather()` de-allocates partially allocated memory on error

- unit test "WeatherFixtureTest, ClimateVariableClimateFromConstantWeather" now use `allocateAllWeather()` and `deallocateAllWeather()`
- Encapsulate RNG variable in dedicated type

# Conflicts:
#	include/rands.h
#	src/rands.c
#	tests/gtests/test_SW_Flow_Lib.cc
#	tests/gtests/test_SW_Flow_Lib_PET.cc
#	tests/gtests/test_SW_Flow_lib_temp.cc
#	tests/gtests/test_rands.cc
- simplify code
- document
- unit tests
Feature error handling

* close #368 "Error handing"
* close #359 "Prevent memory leaks during error/early exist"

* the code now no longer crashes on error (except for STEPWAT2)
* the code now stores messages of warnings and error status
* all functions now check for errors and return early (after cleaning up memory)
- also: clarify contributions for v7.1.0
@dschlaep dschlaep requested a review from N1ckP3rsl3y October 17, 2023 13:01
@dschlaep dschlaep marked this pull request as ready for review October 18, 2023 16:16
N1ckP3rsl3y
N1ckP3rsl3y previously approved these changes Oct 19, 2023
- move `leaks` command from "tools/check_SOILWAT2.sh" to dedicated scripts
- make gains "bin_leaks" and "test_leaks"
- previous "leak" targets renamed to "sanitizer"
- clarify usage of make targets
- clarify the compilation of the simulation executable uses a C toolchain whereas the test executable uses a C++ toolchain
N1ckP3rsl3y and others added 2 commits October 24, 2023 00:21
- The function, `strtok()` is not thread-safe
- Using `strtok_s()` may require undesired changes due to it requiring C11
- The custom function provides the same functionality as `strtok()`	* Created a helper function `isDelim()` -- Handles the determination of it a character is in the list of possible delimiters

- Replaced usage of `strtok()` with the new function

- New test unit tests for the new function
	* Covers empty, one, and multiple of
		* Possible input strings
		* Possible input delimiter strings
- close #376
- update NEWS
- additional unit tests as used by the code
- `MkDir()` can now be simplified thanks to new `sw_strtok()`
- `_create_filename_ST()`: use `Str_Dup()`
@dschlaep dschlaep merged commit 5b236ec into master Oct 24, 2023
@dschlaep dschlaep deleted the release/devel_v7.2.0 branch October 24, 2023 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants