From 481caba465b00445021c7557d611cd5592f22312 Mon Sep 17 00:00:00 2001 From: Alomir Date: Fri, 25 Apr 2025 12:00:00 -0400 Subject: [PATCH 1/4] Revamps .clang-tidy to be a (small) exclusion list instead of (large) inclusion list, with updates to code --- .clang-tidy | 151 +---------------------------------- src/common/exitCodes.h | 2 +- src/common/paramchange.c | 15 ++++ src/common/paramchange.h | 12 +++ src/common/util.c | 2 +- src/estimate/ml-metrorun.c | 2 +- src/sipnet/modelStructures.h | 2 +- src/sipnet/sipnet.c | 5 +- src/utilities/transpose.c | 11 +++ tests/utils/exitHandler.c | 3 +- tests/utils/tUtils.h | 9 +++ 11 files changed, 60 insertions(+), 154 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index d1f3248db..15588b8fe 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,150 +1,7 @@ # Generated from CLion Inspection settings --- -Checks: '-*, -bugprone-argument-comment, -bugprone-assert-side-effect, -bugprone-bad-signal-to-kill-thread, -bugprone-branch-clone, -bugprone-copy-constructor-init, -bugprone-dangling-handle, -bugprone-dynamic-static-initializers, -bugprone-fold-init-type, -bugprone-forward-declaration-namespace, -bugprone-forwarding-reference-overload, -bugprone-inaccurate-erase, -bugprone-incorrect-roundings, -bugprone-integer-division, -bugprone-lambda-function-name, -bugprone-macro-parentheses, -bugprone-macro-repeated-side-effects, -bugprone-misplaced-operator-in-strlen-in-alloc, -bugprone-misplaced-pointer-arithmetic-in-alloc, -bugprone-misplaced-widening-cast, -bugprone-move-forwarding-reference, -bugprone-multiple-statement-macro, -bugprone-no-escape, -bugprone-parent-virtual-call, -bugprone-posix-return, -bugprone-reserved-identifier, -bugprone-sizeof-container, -bugprone-sizeof-expression, -bugprone-spuriously-wake-up-functions, -bugprone-string-constructor, -bugprone-string-integer-assignment, -bugprone-string-literal-with-embedded-nul, -bugprone-suspicious-enum-usage, -bugprone-suspicious-include, -bugprone-suspicious-memset-usage, -bugprone-suspicious-missing-comma, -bugprone-suspicious-semicolon, -bugprone-suspicious-string-compare, -bugprone-suspicious-memory-comparison, -bugprone-suspicious-realloc-usage, -bugprone-swapped-arguments, -bugprone-terminating-continue, -bugprone-throw-keyword-missing, -bugprone-too-small-loop-variable, -bugprone-undefined-memory-manipulation, -bugprone-undelegated-constructor, -bugprone-unhandled-self-assignment, -bugprone-unused-raii, -bugprone-unused-return-value, -bugprone-use-after-move, -bugprone-virtual-near-miss, -cert-dcl21-cpp, -cert-dcl58-cpp, -#cert-err34-c, -cert-err52-cpp, -cert-err60-cpp, -cert-flp30-c, -#cert-msc50-cpp, -#cert-msc51-cpp, -cert-str34-c, -cppcoreguidelines-avoid-shadowing, -cppcoreguidelines-interfaces-global-init, -cppcoreguidelines-pro-type-member-init, -cppcoreguidelines-pro-type-static-cast-downcast, -cppcoreguidelines-slicing, -google-default-arguments, -google-explicit-constructor, -google-runtime-operator, -hicpp-exception-baseclass, -hicpp-multiway-paths-covered, -misc-misplaced-const, -misc-new-delete-overloads, -misc-no-recursion, -misc-non-copyable-objects, -misc-throw-by-value-catch-by-reference, -misc-unconventional-assign-operator, -misc-uniqueptr-reset-release, -modernize-avoid-bind, -modernize-concat-nested-namespaces, -modernize-deprecated-headers, -modernize-deprecated-ios-base-aliases, -modernize-loop-convert, -modernize-make-shared, -modernize-make-unique, -modernize-pass-by-value, -modernize-raw-string-literal, -modernize-redundant-void-arg, -modernize-replace-auto-ptr, -modernize-replace-disallow-copy-and-assign-macro, -modernize-replace-random-shuffle, -modernize-return-braced-init-list, -modernize-shrink-to-fit, -modernize-unary-static-assert, -modernize-use-auto, -modernize-use-bool-literals, -modernize-use-emplace, -modernize-use-equals-default, -modernize-use-equals-delete, -modernize-use-nodiscard, -modernize-use-noexcept, -modernize-use-nullptr, -modernize-use-override, -modernize-use-transparent-functors, -modernize-use-uncaught-exceptions, -mpi-buffer-deref, -mpi-type-mismatch, -openmp-use-default-none, -performance-faster-string-find, -performance-for-range-copy, -performance-implicit-conversion-in-loop, -performance-inefficient-algorithm, -performance-inefficient-string-concatenation, -performance-inefficient-vector-operation, -performance-move-const-arg, -performance-move-constructor-init, -performance-no-automatic-move, -performance-noexcept-move-constructor, -performance-trivially-destructible, -performance-type-promotion-in-math-fn, -performance-unnecessary-copy-initialization, -performance-unnecessary-value-param, -portability-simd-intrinsics, -readability-avoid-const-params-in-decls, -readability-braces-around-statements, -readability-const-return-type, -readability-container-size-empty, -readability-convert-member-functions-to-static, -readability-delete-null-pointer, -readability-deleted-default, -readability-inconsistent-declaration-parameter-name, -#readability-magic-numbers, -readability-make-member-function-const, -#readability-math-missing-parentheses, -readability-misleading-indentation, -readability-misplaced-array-index, -readability-redundant-control-flow, -readability-redundant-declaration, -readability-redundant-function-ptr-dereference, -readability-redundant-smartptr-get, -readability-redundant-string-cstr, -readability-redundant-string-init, -readability-simplify-subscript-expr, -readability-static-accessed-through-instance, -readability-static-definition-in-anonymous-namespace, -readability-string-compare, -readability-uniqueptr-delete-release, -readability-use-anyofallof' +Checks: ' +-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling, +-clang-analyzer-unix.Errno, +-clang-analyzer-security.insecureAPI.rand' WarningsAsErrors: '*' diff --git a/src/common/exitCodes.h b/src/common/exitCodes.h index 10fccc92c..b189b2175 100644 --- a/src/common/exitCodes.h +++ b/src/common/exitCodes.h @@ -20,7 +20,7 @@ typedef enum { EXIT_CODE_BAD_PARAMETER_VALUE = 3, EXIT_CODE_UNKNOWN_EVENT_TYPE_OR_PARAM = 4, EXIT_CODE_INPUT_FILE_ERROR = 5, - EXIT_CODE_FILE_OPEN_ERROR = 6, + EXIT_CODE_FILE_OPEN_OR_READ_ERROR = 6, } exit_code_t; #endif diff --git a/src/common/paramchange.c b/src/common/paramchange.c index 253ba8972..c2c211437 100644 --- a/src/common/paramchange.c +++ b/src/common/paramchange.c @@ -398,9 +398,13 @@ void computeAggedData(double **theAggedData, double **origArray, above] */ +// Apr-25-2025: clang-tidy asserts that there are issues with this function, +// and it is not currently being used. Disabling for now, not worth +// spending time on. // 6/9/11: For now we don't do anything with the cost function or data Type // weights - we just want to make it consistent with the difference function // above so we don't get an error. +#if 0 double aggedDifference(double *sigma, OutputInfo *outputInfo, int loc, SpatialParams *spatialParams, double paramWeight, void (*modelF)(double **, int, int *, SpatialParams *, @@ -492,6 +496,7 @@ double aggedDifference(double *sigma, OutputInfo *outputInfo, int loc, return logLike; } +#endif /* Take array of model output, compare output with measured data - for given dataNum (i.e. perform comparisons between model[*][dataNum] and @@ -679,6 +684,9 @@ void readIndicesFile(char *fileName, int *startIndices, int *endIndices, } } +// Apr-25-2025: clang-tidy asserts that there are issues with this function, +// and it is not currently being used. Disabling for now, not worth +// spending time on. /* Read measured data (from fileName.dat) and valid fractions (from fileName.valid) into arrays (used to also read sigmas) and set values in valid array (based on validFrac) Each line in data (and valid) file has @@ -707,6 +715,7 @@ void readIndicesFile(char *fileName, int *startIndices, int *endIndices, This function also allocates space for model array */ +#if 0 void readData(char *fileName, int dataTypeIndices[], int numDataTypes, int totNumDataTypes, int myNumLocs, int *steps, double validFrac, char *optIndicesFile, char *compareIndicesFile, FILE *outFile) { @@ -993,7 +1002,11 @@ void readData(char *fileName, int dataTypeIndices[], int numDataTypes, fclose(in1); } +#endif +// Apr-25-2025: clang-tidy asserts that there are issues with this function, +// and it is not currently being used. Disabling for now, not worth +// spending time on. /* pre: readData has been called (to set global startOpt, endOpt and numLocs appropriately) @@ -1012,6 +1025,7 @@ void readData(char *fileName, int dataTypeIndices[], int numDataTypes, and unaggedWeight appropriately also compute aggedData array (of size numLocs x numAggSteps x numDataTypes) */ +#if 0 void readFileForAgg(char *fileForAgg, int numDataTypes, double myUnaggedWeight) { FILE *f; @@ -1071,6 +1085,7 @@ void readFileForAgg(char *fileForAgg, int numDataTypes, aggedModel = make2DArray(maxCount, numDataTypes); unaggedWeight = myUnaggedWeight; } +#endif // malloc space for outputInfo array[0..numDataTypes-1], and outputInfo[*].years // arrays for a single location, loc make years arrays large enough to hold data diff --git a/src/common/paramchange.h b/src/common/paramchange.h index e789f5bd9..893c865ca 100644 --- a/src/common/paramchange.h +++ b/src/common/paramchange.h @@ -38,6 +38,9 @@ typedef struct OutputInfoStruct { NOTE: this is actually the NEGATIVE log likelihood, discarding constant terms to get true log likelihood, add n*log(sqrt(2*pi)), then multiply by -1 */ +// Function is disabled, SHOULD comment out here as well, but that breaks +// compilation of estimate/ml-metrorun.c - which we don't care about and should +// remove, but that's something for another day... double difference(double *sigma, OutputInfo *outputInfo, int loc, SpatialParams *spatialParams, double paramWeight, void (*modelF)(double **, int, int *, SpatialParams *, int), @@ -81,6 +84,9 @@ double difference(double *sigma, OutputInfo *outputInfo, int loc, // weights - we just want to make it consistent with the difference function // above so we don't get an error. +// Function is disabled, SHOULD comment out here as well, but that breaks +// compilation of estimate/ml-metrorun.c - which we don't care about and should +// remove, but that's something for another day... double aggedDifference(double *sigma, OutputInfo *outputInfo, int loc, SpatialParams *spatialParams, double paramWeight, void (*modelF)(double **, int, int *, SpatialParams *, @@ -128,6 +134,9 @@ void aggregates(OutputInfo *outputInfo, double **localModel, int loc, This function also allocates space for model array */ +// Function is disabled, SHOULD comment out here as well, but that breaks +// compilation of estimate/ml-metrorun.c - which we don't care about and should +// remove, but that's something for another day... void readData(char *fileName, int dataTypeIndices[], int numDataTypes, int totNumDataTypes, int myNumLocs, int *steps, double validFrac, char *optIndicesFile, char *compareIndicesFile, FILE *outFile); @@ -150,6 +159,9 @@ void readData(char *fileName, int dataTypeIndices[], int numDataTypes, and unaggedWeight appropriately also compute aggedData array (of size numLocs x numAggSteps x numDataTypes) */ +// Function is disabled, SHOULD comment out here as well, but that breaks +// compilation of estimate/ml-metrorun.c - which we don't care about and should +// remove, but that's something for another day... void readFileForAgg(char *fileForAgg, int numDataTypes, double myUnaggedWeight); // malloc space for outputInfo array[0..numDataTypes-1], and outputInfo[*].years diff --git a/src/common/util.c b/src/common/util.c index ae2a2c150..d30166eb5 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -28,7 +28,7 @@ FILE *openFile(const char *name, const char *mode) { if ((f = fopen(name, mode)) == NULL) { printf("Error opening %s for %s\n", name, mode); - exit(EXIT_CODE_FILE_OPEN_ERROR); + exit(EXIT_CODE_FILE_OPEN_OR_READ_ERROR); } return f; diff --git a/src/estimate/ml-metrorun.c b/src/estimate/ml-metrorun.c index 1d015d36c..cfdd15cef 100644 --- a/src/estimate/ml-metrorun.c +++ b/src/estimate/ml-metrorun.c @@ -204,7 +204,7 @@ int main(int argc, char *argv[]) { int i; // initialize array to [0,1,...,MAX_DATA_TYPES-1] (default) - numDataTypes = initDataTypeIndices(dataTypeIndices); + initDataTypeIndices(dataTypeIndices); // return value not used // get command-line arguments: while ((option = getopt(argc, argv, "hi:")) != -1) { diff --git a/src/sipnet/modelStructures.h b/src/sipnet/modelStructures.h index 46f7bd8dc..5a5351afa 100644 --- a/src/sipnet/modelStructures.h +++ b/src/sipnet/modelStructures.h @@ -19,7 +19,7 @@ // Read in and process agronomic events. SIPNET expects a file named // events.in to exist, though unit tests may use other names. // Note: requires ROOTS to be activated. -#define EVENT_HANDLER 0 && ROOTS +#define EVENT_HANDLER 1 && ROOTS // have extra litter pool, in addition to soil c pool #define LITTER_POOL 0 diff --git a/src/sipnet/sipnet.c b/src/sipnet/sipnet.c index 95c985ef8..f463b351e 100644 --- a/src/sipnet/sipnet.c +++ b/src/sipnet/sipnet.c @@ -1439,7 +1439,10 @@ void moisture(double *trans, double *dWater, double potGrossPsn, double vpd, int pastLeafGrowth(void) { #if GDD - return (climate->gdd >= params.gddLeafOn); // growing degree days threshold + // null pointer dereference warning suppressed on the next line + [[clang::suppress]] return (climate->gdd >= params.gddLeafOn); // growing + // degree days + // threshold #elif SOIL_PHENOL return (climate->tsoil >= params.soilTempLeafOn); // soil temperature // threshold diff --git a/src/utilities/transpose.c b/src/utilities/transpose.c index c9540ed04..4927c8cd4 100644 --- a/src/utilities/transpose.c +++ b/src/utilities/transpose.c @@ -62,6 +62,8 @@ void readFile(FILE *f, char **lines, int maxLength) { int i; i = 0; + // clang-tidy finds this sketchy, but it does seem to work, so ignore + [[clang::suppress]] while (fgets(lines[i], maxLength, f) != NULL) { // while not EOF or error i++; } @@ -155,6 +157,10 @@ int main(int argc, char *argv[]) { f = openFile(filename, "r"); nl = numlines(f, &longestLine); + if (nl < 1) { + printf("Error: input file must contain at least one line\n"); + exit(1); + } rewind(f); // Allocate space to hold the file, line by line: @@ -168,6 +174,10 @@ int main(int argc, char *argv[]) { // Allocate space to hold the individual items: numItems = countItems(lines[0]); + if (numItems < 1) { + printf("Error in input file, first line has no items\n"); + exit(1); + } items = (char ***)malloc(nl * sizeof(char **)); for (i = 0; i < nl; i++) { items[i] = (char **)malloc(numItems * sizeof(char *)); @@ -192,6 +202,7 @@ int main(int argc, char *argv[]) { } free(items); free(lines); + free(tmpName); return 0; } diff --git a/tests/utils/exitHandler.c b/tests/utils/exitHandler.c index 8fc56a778..4343ac439 100644 --- a/tests/utils/exitHandler.c +++ b/tests/utils/exitHandler.c @@ -1,8 +1,7 @@ +#include #include #include -// - static int expected_code = 1; // the expected value a tested function passes to exit static int should_exit = 1; // set in test code; 1 if exit should have been called static int really_exit = 0; // set to 1 to prevent stubbing behavior and actually exit diff --git a/tests/utils/tUtils.h b/tests/utils/tUtils.h index 54f3429c7..30f807d86 100644 --- a/tests/utils/tUtils.h +++ b/tests/utils/tUtils.h @@ -3,6 +3,8 @@ #include #include +#include +#include "common/exitCodes.h" extern inline int copyFile(char* src, char* dest) { FILE *source = fopen(src, "rb"); // Binary mode for compatibility @@ -22,7 +24,14 @@ extern inline int copyFile(char* src, char* dest) { size_t bytesRead; while ((bytesRead = fread(buffer, 1, sizeof(buffer), source)) > 0) { + if (ferror(source)) { + printf("Error reading file %s during file copy\n", src); + exit(EXIT_CODE_FILE_OPEN_OR_READ_ERROR); + } fwrite(buffer, 1, bytesRead, destination); + if (feof(source)) { + break; + } } fclose(source); From 945c03ebf95aa5cbaa895eb68b67bfe5dbc86e10 Mon Sep 17 00:00:00 2001 From: Alomir Date: Fri, 25 Apr 2025 12:14:35 -0400 Subject: [PATCH 2/4] Changes [[clang::suppress]] to NOLINT for portability --- src/sipnet/sipnet.c | 4 +--- src/utilities/transpose.c | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/sipnet/sipnet.c b/src/sipnet/sipnet.c index f463b351e..f6640cfcf 100644 --- a/src/sipnet/sipnet.c +++ b/src/sipnet/sipnet.c @@ -1440,9 +1440,7 @@ int pastLeafGrowth(void) { #if GDD // null pointer dereference warning suppressed on the next line - [[clang::suppress]] return (climate->gdd >= params.gddLeafOn); // growing - // degree days - // threshold + return (climate->gdd >= params.gddLeafOn); // NOLINT #elif SOIL_PHENOL return (climate->tsoil >= params.soilTempLeafOn); // soil temperature // threshold diff --git a/src/utilities/transpose.c b/src/utilities/transpose.c index 4927c8cd4..47f283841 100644 --- a/src/utilities/transpose.c +++ b/src/utilities/transpose.c @@ -63,8 +63,8 @@ void readFile(FILE *f, char **lines, int maxLength) { i = 0; // clang-tidy finds this sketchy, but it does seem to work, so ignore - [[clang::suppress]] - while (fgets(lines[i], maxLength, f) != NULL) { // while not EOF or error + while (fgets(lines[i], maxLength, f) != NULL) { // NOLINT + // while not EOF or error i++; } } From dd78bcb54ec5e9b054074ad54bc33261eebb9aeb Mon Sep 17 00:00:00 2001 From: Alomir Date: Fri, 25 Apr 2025 12:17:28 -0400 Subject: [PATCH 3/4] Fixes unused param warning --- src/common/paramchange.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/common/paramchange.c b/src/common/paramchange.c index c2c211437..f0a0b6b99 100644 --- a/src/common/paramchange.c +++ b/src/common/paramchange.c @@ -45,9 +45,11 @@ static double **aggedModel = NULL; /* aggregated model (intitialized to NULL b/c we may never malloc this array) made global so don't have to re-allocate memory all the time only holds model output between startOpt and endOpt */ -static double unaggedWeight = 0.0; /* if aggregation is done, weight of - unaggregated data in optimization (0 -> only use aggregated data) - */ +// Commenting out as all uses are commented out +// static double unaggedWeight = 0.0; /* if aggregation is done, weight of +// unaggregated data in optimization (0 -> only use aggregated +// data) +// */ typedef struct AggregateInfoStruct { // set at beginning of program int startPt, endPt; // indices of data point to start and stop at From 7739854c4e0e9e9a65849026903cb261cb482183 Mon Sep 17 00:00:00 2001 From: Alomir Date: Fri, 25 Apr 2025 12:20:11 -0400 Subject: [PATCH 4/4] Updates formatting for clang-format --- tests/utils/exitHandler.c | 19 ++++++++++--------- tests/utils/tUtils.h | 10 +++++----- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/tests/utils/exitHandler.c b/tests/utils/exitHandler.c index 4343ac439..6993d2776 100644 --- a/tests/utils/exitHandler.c +++ b/tests/utils/exitHandler.c @@ -2,9 +2,12 @@ #include #include -static int expected_code = 1; // the expected value a tested function passes to exit -static int should_exit = 1; // set in test code; 1 if exit should have been called -static int really_exit = 0; // set to 1 to prevent stubbing behavior and actually exit +static int expected_code = 1; // the expected value a tested function passes to + // exit +static int should_exit = 1; // set in test code; 1 if exit should have been + // called +static int really_exit = 0; // set to 1 to prevent stubbing behavior and + // actually exit static jmp_buf jump_env; @@ -32,13 +35,11 @@ static int exit_result = 1; // test_assert(jmp_rval==1); // stub function -void exit(int code) -{ - if (!really_exit) - { +void exit(int code) { + if (!really_exit) { printf("Mocking the exit call\n"); - test_assert(should_exit==1); - test_assert(expected_code==code); + test_assert(should_exit == 1); + test_assert(expected_code == code); longjmp(jump_env, 1); } diff --git a/tests/utils/tUtils.h b/tests/utils/tUtils.h index 30f807d86..e82ec47ad 100644 --- a/tests/utils/tUtils.h +++ b/tests/utils/tUtils.h @@ -6,8 +6,8 @@ #include #include "common/exitCodes.h" -extern inline int copyFile(char* src, char* dest) { - FILE *source = fopen(src, "rb"); // Binary mode for compatibility +extern inline int copyFile(char *src, char *dest) { + FILE *source = fopen(src, "rb"); // Binary mode for compatibility if (source == NULL) { printf("Error opening source file %s", src); return 1; @@ -20,7 +20,7 @@ extern inline int copyFile(char* src, char* dest) { return 1; } - char buffer[4096]; // 4KB buffer + char buffer[4096]; // 4KB buffer size_t bytesRead; while ((bytesRead = fread(buffer, 1, sizeof(buffer), source)) > 0) { @@ -40,7 +40,7 @@ extern inline int copyFile(char* src, char* dest) { } extern inline int compareDoubles(double a, double b) { - return fabs(a-b) < 1e-6; + return fabs(a - b) < 1e-6; } -#endif //UTILS_H +#endif // UTILS_H