feat(core,cpp): add cleed_core library and cleed++ bindings#84
feat(core,cpp): add cleed_core library and cleed++ bindings#84Liam-Deacon wants to merge 27 commits intomasterfrom
Conversation
Add cleed_core library target with a minimal core API header and version helper. Wire new subdirectory into CMake + autotools builds. Tests: cmake -S . -B build -DCMAKE_BUILD_TYPE=Release; cmake --build build --target cleedCore; python3 -m pre_commit run --all-files
Add in-memory curve helpers (alloc/free/copy/interp) to cleed_core. Introduce a stub cleed_io library layer for future file parsing. Wire new targets into CMake + autotools builds. Tests: cmake -S . -B build -DCMAKE_BUILD_TYPE=Release; cmake --build build --target cleedCore cleedIO; python3 -m pre_commit run --all-files
Port the cleed++ sources from develop, gate the build with BUILD_CPP_BINDINGS, and add a small C++ smoke executable. Ensure C headers are C++-safe and document the experimental C++ API.
Sphinx requires lowercase directive names. The uppercase TODO caused 'Unknown directive type' errors during documentation builds.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Liam-Deacon, your pull request is larger than the review limit of 150000 diff characters
|
Warning Rate limit exceeded@Liam-Deacon has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds an optional CMake flag Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (configure/build)
participant CMake as CMake
participant CLib as C libraries (cleedCore/cleedIO/rfac)
participant CPP as libcleed++ (C++ bindings)
participant EXE as cleedpp_smoke
Dev->>CMake: run cmake -DBUILD_CPP_BINDINGS=ON
CMake->>CLib: configure & build cleedCore, cleedIO, rfac
CMake->>CPP: when BUILD_CPP_BINDINGS ON -> configure libcleed++ targets
CPP->>CLib: link against rfac, cleedCore, search
CMake->>EXE: build cleedpp_smoke linked to libcleed++ and search
EXE->>CLib: runtime interop via C ABI (headers with extern "C")
EXE-->>Dev: smoke test runs (returns 0)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codacy's Analysis Summary100 new issues (≤ 0 issue) Review Pull Request in Codacy →
|
- Remove 'Numerical Recipes' references from spline comments to pass NR compliance guard - Escape <flag> as \<flag\> in rfrdargs.c to fix Doxygen parsing error
Escape <placeholder> style text in comments to prevent Doxygen from treating them as unsupported XML/HTML tags.
Replace backslash-escaped angle brackets with HTML entities (< >) to avoid Doxygen interpreting them as command sequences.
Replace HTML entities and angle-bracket placeholders with simple uppercase identifiers to prevent Doxygen parsing errors.
Remove @code block and \retval \c NULL syntax that was causing TK_COMMAND_BS parser errors in Doxygen.
- Remove inline keyword from rfactor_name/rfactor_print functions in rfac.c to fix MSVC unresolved external symbol error (Windows) - Move ctr_stream/inf_stream definitions from latt.c to latt_streams.c to fix multiple definition linker error when latt_core objects are linked into both latt executable and cleed++ library (Ubuntu) - Fix Doxygen syntax error: replace problematic '\retval \c NULL' with inline description in rfrdexpt.c
- Fix Doxygen @param documentation to match actual parameter names: - rfspline.c: *list -> iv - rfsplint.c: *list -> list - Remove duplicate ctr_stream/inf_stream definitions from test files to fix multiple definition linker errors when linking with latt_core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 222
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/include/matrix_2x2.h (4)
50-50: Removeexplicitkeyword from copy constructor.Copy constructors should not be marked
explicit. Theexplicitkeyword prevents implicit conversions, but copy construction is intended to work implicitly in C++. This violates standard C++ practices and may cause compilation errors or unexpected behavior.🐛 Proposed fix
- Matrix2x2(const Matrix2x2 &Mat) : mat (::matrix_2x2_copy(Mat.mat)) {} + Matrix2x2(const Matrix2x2 &Mat) : mat(::matrix_2x2_copy(Mat.mat)) {}
57-57: Inefficient return type:const std::vector<double>.Returning a
constqualified value type is unconventional and provides no benefit. Theconstapplies to the temporary returned object, which the caller cannot modify anyway. Additionally, returningstd::vectorby value can be expensive for large vectors.♻️ Recommended fix
Consider one of these alternatives:
Option 1 (preferred): Return by value without const
- const std::vector<double> getAllMatrixElements(); + std::vector<double> getAllMatrixElements() const;Option 2: Return by const reference (if the vector is stored internally)
- const std::vector<double> getAllMatrixElements(); + const std::vector<double>& getAllMatrixElements() const;Note: Both options mark the method itself as
constsince it doesn't modify the object.
58-58: Remove unnecessaryconstfrom primitive return type.The
constqualifier on a primitive return type (double) has no effect and is considered poor style in C++. It only applies to the temporary value that the caller receives.♻️ Proposed refactor
- const double getSingleMatrixElement(std::size_t row, std::size_t col); + double getSingleMatrixElement(std::size_t row, std::size_t col) const;Note: Also marked the method as
constsince it doesn't modify the object.
51-58: DefinegetSingleMatrixElement()in the implementation file.
getSingleMatrixElement()is declared on line 58 but has no definition insrc/patt/matrix_2x2.cc. This will cause an undefined reference linker error. The other member functions (setMatrix(),setMatrixElement(),print(),getAllMatrixElements(), and~Matrix2x2()) are properly defined in the implementation file.CMakeLists.txt (1)
75-87: AddBUILD_CPP_BINDINGSto the configuration summary for consistency.The new option is not echoed in the status output like other configuration flags (
WITH_QT,USE_OPENMP,BUILD_SHARED_LIBS,INSTALL_DOC, etc.), making it harder to verify the build configuration in CI logs and debug reports.Suggested patch
MESSAGE( STATUS "WITH_QT = ${WITH_QT}") MESSAGE( STATUS "USE_OPENMP = ${USE_OPENMP}" ) +MESSAGE( STATUS "BUILD_CPP_BINDINGS = ${BUILD_CPP_BINDINGS}" )src/include/lattice.h (1)
144-152: Critical: Missing semicolon after class definition.The
Latticeclass declaration is missing a semicolon after the closing brace, which will cause a compilation error.🔧 Proposed fix
protected: lattice_t *lat; -} +};src/include/pattern.h (1)
188-224: Macro-gating legacy C++ wrapper looks correct; fix wrapper self-containment when enabled.
CLEED_DISABLE_LEGACY_CPPwrapping is a good step, but with legacy enabled this header still appears to rely on undeclaredstring/ missing<string>.Proposed patch
#ifdef __cplusplus /* If this is a C++ compiler, use C linkage */ #include <list> + #include <string> extern "C" { #endif @@ #ifndef CLEED_DISABLE_LEGACY_CPP namespace cleed { @@ - const string getTitle(); + std::string getTitle();src/include/miller_index.h (1)
18-41: Document and centralize theCLEED_DISABLE_LEGACY_CPPswitch to avoid downstream build surprises.
This new guard is fine mechanically, but it needs a single source of truth (config header / build option docs) so consumers know when/where it’s defined and what it disables.src/include/spots.h (2)
211-217: DuplicatesetKmethod declarations.There are two
setKmethod declarations with identical signatures at lines 211 and 217. Additionally, the Doxygen comment at line 213-216 incorrectly referencessetKfor setting Miller indexl, but the parameter is namedh.This appears to be a copy-paste error where
setLwas intended for the second declaration.🐛 Proposed fix
/*! \brief Miller index k setter function. * \sa setK() * \param k Value of Miller index k. */ void setK(double h); - /*! \brief Miller index l setter function. - * \sa setK() - * \param k Value of Miller index . + /*! \brief Miller index l setter function. + * \sa setL() + * \param l Value of Miller index l. */ - void setK(double h); + void setL(double l);
273-273: Missingstd::namespace qualifiers and includes for STL types.The legacy C++ class uses
stringandlist<Spot>without thestd::prefix. This will fail to compile unlessusing namespace std;is present elsewhere (which is bad practice in headers).🐛 Proposed fix
Add the necessary includes after line 186 (inside the
__cplusplusblock but before the namespace):#include <string> #include <list>Then qualify the types:
- void setFontName(string name); + void setFontName(std::string name); ... - void setSpotList(list<Spot> spots); + void setSpotList(std::list<Spot> spots); ... - const string getFontName(); + const std::string getFontName(); - const list<Spot> getSpotList(); + const std::list<Spot> getSpotList();Also applies to: 277-277, 288-288, 290-290
| The ``cleed++`` library is experimental. The ABI and API are not yet guaranteed | ||
| stable and may change between releases. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good “experimental ABI/API” warning; consider adding stability expectations for the C ABI boundary.
E.g., explicitly state whether the C ABI is intended to be stable “within a major release” or “best-effort only” to set user expectations.
🤖 Prompt for AI Agents
In @doc/cpp_bindings.rst around lines 10 - 12, The current note says "cleed++
library is experimental" but doesn't specify expectations for the C ABI
boundary; add one clear sentence after the existing paragraph stating whether
the C ABI is intended to be stable (e.g., "The C ABI will be maintained stable
within a major release") or is best-effort only (e.g., "The C ABI is provided as
best-effort and may change between releases"), and mention how stability maps to
semantic versioning (major version bumps may introduce breaking ABI changes);
update the paragraph around the "cleed++" mention to include that explicit C ABI
stability statement so users know what to expect.
| Enable the C++ library with ``BUILD_CPP_BINDINGS``: | ||
|
|
||
| .. code-block:: console | ||
|
|
||
| cmake -S . -B build -DBUILD_CPP_BINDINGS=ON | ||
| cmake --build build | ||
|
|
||
| This builds: | ||
|
|
||
| - ``libcleed++`` (shared) | ||
| - ``libcleed++`` (static) | ||
| - ``cleedpp_smoke`` (C++ example) | ||
|
|
||
| Install layout | ||
| ============== | ||
|
|
||
| When installed, the C++ headers and libraries are placed alongside the C | ||
| artifacts: | ||
|
|
||
| - Headers: ``include/cleed++`` | ||
| - Libraries: ``lib`` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add an explicit install step + clarify option default and platform naming.
Right now the doc shows configure/build but not install; also “libcleed++ (shared/static)” naming varies by platform/toolchain.
Proposed patch
Enable the C++ library with ``BUILD_CPP_BINDINGS``:
.. code-block:: console
cmake -S . -B build -DBUILD_CPP_BINDINGS=ON
cmake --build build
+ cmake --install build
+``BUILD_CPP_BINDINGS`` defaults to ``OFF``.
+
This builds:
- ``libcleed++`` (shared)
- ``libcleed++`` (static)
- ``cleedpp_smoke`` (C++ example)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Enable the C++ library with ``BUILD_CPP_BINDINGS``: | |
| .. code-block:: console | |
| cmake -S . -B build -DBUILD_CPP_BINDINGS=ON | |
| cmake --build build | |
| This builds: | |
| - ``libcleed++`` (shared) | |
| - ``libcleed++`` (static) | |
| - ``cleedpp_smoke`` (C++ example) | |
| Install layout | |
| ============== | |
| When installed, the C++ headers and libraries are placed alongside the C | |
| artifacts: | |
| - Headers: ``include/cleed++`` | |
| - Libraries: ``lib`` | |
| Enable the C++ library with ``BUILD_CPP_BINDINGS``: | |
| .. code-block:: console | |
| cmake -S . -B build -DBUILD_CPP_BINDINGS=ON | |
| cmake --build build | |
| cmake --install build | |
| ``BUILD_CPP_BINDINGS`` defaults to ``OFF``. | |
| This builds: | |
| - ``libcleed++`` (shared) | |
| - ``libcleed++`` (static) | |
| - ``cleedpp_smoke`` (C++ example) | |
| Install layout | |
| ============== | |
| When installed, the C++ headers and libraries are placed alongside the C | |
| artifacts: | |
| - Headers: ``include/cleed++`` | |
| - Libraries: ``lib`` |
| const char *cleed_core_version(void) | ||
| { | ||
| return SEARCH_VERSION; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify version naming inconsistency.
The function cleed_core_version() returns SEARCH_VERSION, which creates a naming mismatch. This could confuse API consumers about which version is being reported.
Consider one of the following:
- Define a dedicated macro (preferred if versions differ):
// In cleed_core.h or a new cleed_core_ver.h
#define CLEED_CORE_VERSION "1.0.0"
const char *cleed_core_version(void)
{
return CLEED_CORE_VERSION;
}- Document the relationship if CLEED core intentionally tracks SEARCH version:
/**
* Returns the CLEED core library version.
* Note: cleed_core tracks the SEARCH version.
*/
const char *cleed_core_version(void)
{
return SEARCH_VERSION;
}- Rename the function to reflect what it actually returns:
const char *cleed_search_version(void)
{
return SEARCH_VERSION;
}🧰 Tools
🪛 Cppcheck (2.19.0)
[style] 10-10: The function 'cleed_core_version' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In @src/cleed_core/cleed_core.c around lines 10 - 13, The cleed_core_version()
function returns SEARCH_VERSION causing a naming mismatch; define and use a
dedicated macro CLEED_CORE_VERSION (e.g., add CLEED_CORE_VERSION in cleed_core.h
or a new cleed_core_ver.h) and update cleed_core_version() to return
CLEED_CORE_VERSION, and ensure the header declaration matches; alternatively, if
core intentionally tracks SEARCH, add a doc comment above cleed_core_version()
stating it returns SEARCH_VERSION or rename the function to
cleed_search_version() to reflect the actual macro—pick the dedicated macro
approach unless versions must remain linked.
| int cleed_io_read_curve(const char *path, cleed_curve *out_curve) | ||
| { | ||
| (void)path; | ||
| (void)out_curve; | ||
| errno = ENOSYS; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Stub should validate inputs and avoid leaving *out_curve in an indeterminate state on failure.
At minimum, treat NULL pointers as EINVAL; optionally memset/assign a known-empty curve before returning failure.
Proposed patch
int cleed_io_read_curve(const char *path, cleed_curve *out_curve)
{
- (void)path;
- (void)out_curve;
- errno = ENOSYS;
- return -1;
+ if (!path || !out_curve) {
+ errno = EINVAL;
+ return -1;
+ }
+ /* TODO: implement; for now, return "not implemented". */
+ errno = ENOSYS;
+ return -1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int cleed_io_read_curve(const char *path, cleed_curve *out_curve) | |
| { | |
| (void)path; | |
| (void)out_curve; | |
| errno = ENOSYS; | |
| return -1; | |
| } | |
| int cleed_io_read_curve(const char *path, cleed_curve *out_curve) | |
| { | |
| if (!path || !out_curve) { | |
| errno = EINVAL; | |
| return -1; | |
| } | |
| /* TODO: implement; for now, return "not implemented". */ | |
| errno = ENOSYS; | |
| return -1; | |
| } |
🧰 Tools
🪛 Cppcheck (2.19.0)
[style] 12-12: The function 'cleed_io_read_curve' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In @src/cleed_io/cleed_io.c around lines 12 - 18, The stub cleed_io_read_curve
currently ignores parameters and returns ENOSYS; validate inputs by checking if
path == NULL or out_curve == NULL and in that case set errno = EINVAL and return
-1, and before returning on any failure ensure *out_curve is left in a known
empty state (e.g., zeroed or set to an "empty" default) so callers don't observe
an indeterminate object; keep the ENOSYS return for unimplemented behavior only
after input validation and after initializing out_curve when out_curve is
non-NULL.
| int cleed_io_write_curve(const char *path, const cleed_curve *curve) | ||
| { | ||
| (void)path; | ||
| (void)curve; | ||
| errno = ENOSYS; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider matching a consistent error contract: return -1 and set errno (documented).
This is fine as-is, but the header docs should explicitly say “returns -1 and sets errno” if that’s the intended stable contract.
🧰 Tools
🪛 Cppcheck (2.19.0)
[style] 20-20: The function 'cleed_io_write_curve' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In @src/cleed_io/cleed_io.c around lines 20 - 26, The implementation of
cleed_io_write_curve currently returns -1 and sets errno = ENOSYS; update the
public contract in the header/docs for cleed_io_write_curve to explicitly state
that on failure the function returns -1 and sets errno (with ENOSYS used for
unimplemented), and ensure any future implementations preserve this error
behavior; reference the cleed_io_write_curve declaration and errno/ENOSYS in the
doc comment so callers know to check errno on -1.
| #include <limits.h> | ||
|
|
||
| /*! | ||
| * Returns the number of angle inputs as given by 'sa:' commnad string in |
There was a problem hiding this comment.
Fix typo in documentation.
The comment contains a typo: "commnad" should be "command".
📝 Proposed fix
- * Returns the number of angle inputs as given by 'sa:' commnad string in
+ * Returns the number of angle inputs as given by 'sa:' command string in📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Returns the number of angle inputs as given by 'sa:' commnad string in | |
| * Returns the number of angle inputs as given by 'sa:' command string in |
🤖 Prompt for AI Agents
In @src/rfac/rfgetsa.c at line 25, Update the comment that reads "Returns the
number of angle inputs as given by 'sa:' commnad string in" to correct the typo:
change "commnad" to "command" so it reads "Returns the number of angle inputs as
given by 'sa:' command string in"; locate and edit this comment in
src/rfac/rfgetsa.c (the line containing "'sa:' commnad string") to apply the
fix.
| static size_t rfac_ivcur_get_n_leng(const rfac_ivcur *iv_cur, real de) { | ||
| real e_range = 0.; | ||
| size_t n_leng = 0; | ||
| for(size_t i=0; | ||
| #if DEBUG | ||
| &iv_cur[i] != NULL && | ||
| #endif | ||
| iv_cur[i].group_id != END_OF_GROUP_ID; i++) | ||
| { | ||
| fprintf(stderr, "i=%i\n", i); | ||
| #if SHIFT_DE | ||
| e_range = rfac_iv_get_energy_range(iv_cur[i].experimental); | ||
| n_leng = MAX(n_leng, (size_t)abs((int)(e_range/de))); | ||
| e_range = rfac_iv_get_energy_range(iv_cur[i].theory); | ||
| n_leng = MAX(n_leng, (size_t)abs((int)(e_range/de))); | ||
| #else | ||
| n_leng = MAX(n_leng, iv_cur[i].experimental->n_eng); | ||
| n_leng = MAX(n_leng, iv_cur[i].theory->n_eng); | ||
| #endif | ||
| } | ||
| return n_leng; | ||
| } |
There was a problem hiding this comment.
Remove debug code before merging.
Lines 88 and the surrounding debug scaffold should not be in production code. The fprintf(stderr, "i=%i\n", i); on line 88 will print for every dataset during normal operation.
🧹 Proposed cleanup
static size_t rfac_ivcur_get_n_leng(const rfac_ivcur *iv_cur, real de) {
real e_range = 0.;
size_t n_leng = 0;
for(size_t i=0;
-#if DEBUG
- &iv_cur[i] != NULL &&
-#endif
iv_cur[i].group_id != END_OF_GROUP_ID; i++)
{
- fprintf(stderr, "i=%i\n", i);
#if SHIFT_DE
e_range = rfac_iv_get_energy_range(iv_cur[i].experimental);🤖 Prompt for AI Agents
In @src/rfac/rfrmin.c around lines 79 - 100, In rfac_ivcur_get_n_leng remove the
debug scaffold: delete the fprintf(stderr, "i=%i\n", i); and the DEBUG-only loop
condition fragment (#if DEBUG ... &iv_cur[i] != NULL && #endif) so the for loop
uses only iv_cur[i].group_id != END_OF_GROUP_ID as its termination check; keep
the rest of the logic (n_leng calculations) unchanged.
|
|
||
| rfac_iv_print_list(eng, t_int, e_int); | ||
| exit(0); | ||
|
|
There was a problem hiding this comment.
Remove debug exit before merging.
Lines 406-407 call rfac_iv_print_list() for debugging and immediately exit(0), blocking the R-factor minimization loop. This must be removed or gated behind a compile-time flag before merging.
🧹 Remove or gate debug code
n_leng = rfac_iv_make_list(eng, e_int, t_int, args->s_step, shift,
iv_cur[i_list].experimental->data,
iv_cur[i_list].experimental->n_eng,
iv_cur[i_list].theory->data,
iv_cur[i_list].theory->n_eng);
- rfac_iv_print_list(eng, t_int, e_int);
- exit(0);
-
if(n_leng > 1) Or wrap it in #ifdef DEBUG:
+#ifdef DEBUG
rfac_iv_print_list(eng, t_int, e_int);
exit(0);
+#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rfac_iv_print_list(eng, t_int, e_int); | |
| exit(0); | |
| #ifdef DEBUG | |
| rfac_iv_print_list(eng, t_int, e_int); | |
| exit(0); | |
| #endif |
🤖 Prompt for AI Agents
In @src/rfac/rfrmin.c around lines 405 - 408, Remove the debug-induced early
termination: the call to exit(0) immediately after rfac_iv_print_list(eng,
t_int, e_int) prevents the R-factor minimization loop from running; either
delete the exit(0) or gate both the rfac_iv_print_list(...) and exit(0) behind a
compile-time flag (e.g., #ifdef DEBUG / #endif) so normal execution continues
while allowing debug printing when enabled.
| /* Write IV curves with best agreement to files */ | ||
| rfac_ivcur_write(args->iv_file, iv_cur, n_leng, *r_min_ptr, shift, | ||
| *e_range_ptr, e_range, args->r_type, args->vi); | ||
|
|
There was a problem hiding this comment.
Pass the correct shift value to rfac_ivcur_write.
Line 447 passes shift (the last loop iteration value) instead of *s_min_ptr (the shift corresponding to the minimum R-factor). This will cause the written IV curves to use the wrong shift value.
🐛 Proposed fix
/* Write IV curves with best agreement to files */
- rfac_ivcur_write(args->iv_file, iv_cur, n_leng, *r_min_ptr, shift,
+ rfac_ivcur_write(args->iv_file, iv_cur, n_leng, *r_min_ptr, *s_min_ptr,
*e_range_ptr, e_range, args->r_type, args->vi);🤖 Prompt for AI Agents
In @src/rfac/rfrmin.c around lines 446 - 449, The call to rfac_ivcur_write uses
the loop variable shift instead of the best-shift pointer *s_min_ptr; update the
invocation of rfac_ivcur_write to pass *s_min_ptr (the shift corresponding to
*r_min_ptr) so the written IV curves use the shift that produced the minimum
R-factor; locate the call to rfac_ivcur_write and replace the shift argument
with *s_min_ptr while keeping the other arguments (args->iv_file, iv_cur,
n_leng, *r_min_ptr, e_range, *e_range_ptr, args->r_type, args->vi) unchanged.
| real rfac_splint(real eng, rfac_iv_data *list, size_t leng) | ||
| { | ||
| real y; | ||
| int klo, khi, k; | ||
| real h, b, a; | ||
|
|
||
| /* Find the right place in list by means of bisection. */ | ||
| klo = 0; | ||
| khi = (int)(leng)-1; | ||
| while (khi-klo > 1) | ||
| { | ||
| k = (khi+klo) >> 1; /* integer division by 2 */ | ||
| if (list[k].energy > eng) khi = k; | ||
| else klo = k; | ||
| } | ||
|
|
||
| h = list[khi].energy - list[klo].energy; | ||
| if (IS_EQUAL_REAL(h, 0.0)) | ||
| { | ||
| ERROR_MSG("Bad list input\n"); | ||
| exit(RFAC_BAD_IV_DATA); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing precondition checks cause UB for leng < 2 or list == NULL.
As written, leng == 0 makes khi = -1 and dereferences out-of-bounds.
Proposed patch
real rfac_splint(real eng, rfac_iv_data *list, size_t leng)
{
real y;
- int klo, khi, k;
+ size_t klo, khi, k;
real h, b, a;
+ if (!list || leng < 2) {
+ ERROR_MSG("Bad list input\n");
+ exit(RFAC_BAD_IV_DATA);
+ }
+
/* Find the right place in list by means of bisection. */
klo = 0;
- khi = (int)(leng)-1;
- while (khi-klo > 1)
+ khi = leng - 1;
+ while (khi - klo > 1)
{
- k = (khi+klo) >> 1; /* integer division by 2 */
- if (list[k].energy > eng) khi = k;
- else klo = k;
+ k = (khi + klo) >> 1; /* integer division by 2 */
+ if (list[k].energy > eng) {
+ khi = k;
+ } else {
+ klo = k;
+ }
}🧰 Tools
🪛 Clang (14.0.6)
[warning] 50-50: multiple declarations in a single statement reduces readability
(readability-isolate-declaration)
[warning] 50-50: variable 'klo' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable 'khi' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable 'k' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable name 'k' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 59-59: statement should be inside braces
(readability-braces-around-statements)
[warning] 60-60: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.19.0)
[style] 47-47: The function 'rfac_splint' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In @src/rfac/rfsplint.c around lines 47 - 69, In rfac_splint, add precondition
checks at the top to ensure list is not NULL and leng >= 2 before using klo/khi;
if either check fails, call ERROR_MSG with a clear message and
exit(RFAC_BAD_IV_DATA) (matching the existing error handling), so the bisection
and later dereferences of list[klo]/list[khi] cannot cause undefined behavior.
| <?xml version="1.0" encoding="UTF-8" standalone="no"?> | ||
| <?fileVersion 4.0.0?><cproject storage_type_id="org.eclipse.cdt.core.XmlProjectDescriptionStorage"> | ||
| <storageModule moduleId="org.eclipse.cdt.core.settings"> | ||
| <cconfiguration id="cdt.managedbuild.toolchain.gnu.mingw.base.868482903"> | ||
| <storageModule buildSystemId="org.eclipse.cdt.managedbuilder.core.configurationDataProvider" id="cdt.managedbuild.toolchain.gnu.mingw.base.868482903" moduleId="org.eclipse.cdt.core.settings" name="Debug.lib.MinGW"> | ||
| <externalSettings> | ||
| <externalSetting> | ||
| <entry flags="VALUE_WORKSPACE_PATH" kind="includePath" name="/c++"/> | ||
| <entry flags="VALUE_WORKSPACE_PATH" kind="includePath" name="/cleed++"/> | ||
| <entry flags="VALUE_WORKSPACE_PATH" kind="libraryPath" name="/c++/Default"/> | ||
| <entry flags="VALUE_WORKSPACE_PATH" kind="libraryPath" name="/cleed++/Default"/> | ||
| <entry flags="VALUE_WORKSPACE_PATH" kind="libraryPath" name="/cleed++/Debug.lib.MinGW"/> | ||
| <entry flags="RESOLVED" kind="libraryFile" name="cleed++" srcPrefixMapping="" srcRootPath=""/> | ||
| </externalSetting> | ||
| </externalSettings> | ||
| <extensions> | ||
| <extension id="org.eclipse.cdt.core.PE" point="org.eclipse.cdt.core.BinaryParser"/> | ||
| <extension id="org.eclipse.cdt.core.GCCErrorParser" point="org.eclipse.cdt.core.ErrorParser"/> | ||
| <extension id="org.eclipse.cdt.core.GASErrorParser" point="org.eclipse.cdt.core.ErrorParser"/> | ||
| <extension id="org.eclipse.cdt.core.GLDErrorParser" point="org.eclipse.cdt.core.ErrorParser"/> | ||
| </extensions> | ||
| </storageModule> | ||
| <storageModule moduleId="cdtBuildSystem" version="4.0.0"> | ||
| <configuration artifactExtension="" artifactName="cleed++" buildArtefactType="org.eclipse.cdt.build.core.buildArtefactType.sharedLib" buildProperties="org.eclipse.cdt.build.core.buildArtefactType=org.eclipse.cdt.build.core.buildArtefactType.sharedLib" description="Windows DLL" errorParsers="org.eclipse.cdt.core.GCCErrorParser;org.eclipse.cdt.core.GASErrorParser;org.eclipse.cdt.core.GLDErrorParser" id="cdt.managedbuild.toolchain.gnu.mingw.base.868482903" name="Debug.lib.MinGW" parent="org.eclipse.cdt.build.core.emptycfg" postannouncebuildStep="" postbuildStep="" preannouncebuildStep="" prebuildStep=""> | ||
| <folderInfo id="cdt.managedbuild.toolchain.gnu.mingw.base.868482903.474468868" name="/" resourcePath=""> | ||
| <toolChain errorParsers="" id="cdt.managedbuild.toolchain.gnu.mingw.base.172836607" name="cdt.managedbuild.toolchain.gnu.mingw.base" superClass="cdt.managedbuild.toolchain.gnu.mingw.base" unusedChildren="cdt.managedbuild.tool.gnu.c.compiler.mingw.base"> | ||
| <targetPlatform archList="all" binaryParser="org.eclipse.cdt.core.PE" id="cdt.managedbuild.target.gnu.platform.mingw.base.1374643539" name="Debug Platform" osList="win32" superClass="cdt.managedbuild.target.gnu.platform.mingw.base"/> | ||
| <builder buildPath="${workspace_loc:/c++}/Default" errorParsers="" id="cdt.managedbuild.tool.gnu.builder.mingw.base.747379364" keepEnvironmentInBuildfile="false" managedBuildOn="false" name="CDT Internal Builder" superClass="cdt.managedbuild.tool.gnu.builder.mingw.base"/> | ||
| <tool command="g++" commandLinePattern="${COMMAND} ${FLAGS} ${OUTPUT_FLAG} ${OUTPUT_PREFIX}${OUTPUT} ${INPUTS}" errorParsers="org.eclipse.cdt.core.GCCErrorParser" id="cdt.managedbuild.tool.gnu.cpp.compiler.mingw.base.101507346" name="GCC C++ Compiler" superClass="cdt.managedbuild.tool.gnu.cpp.compiler.mingw.base"> | ||
| <option id="gnu.cpp.compiler.option.include.paths.1301976395" name="Include paths (-I)" superClass="gnu.cpp.compiler.option.include.paths" useByScannerDiscovery="false" valueType="includePath"> | ||
| <listOptionValue builtIn="false" value=""${workspace_loc:/CLEED/src/include}""/> | ||
| <listOptionValue builtIn="false" value=""${workspace_loc:/c++}""/> | ||
| <listOptionValue builtIn="false" value=""${workspace_loc:/cleed++}""/> | ||
| </option> | ||
| <option id="gnu.cpp.compiler.option.dialect.std.1153707105" name="Language standard" superClass="gnu.cpp.compiler.option.dialect.std" useByScannerDiscovery="true" value="gnu.cpp.compiler.dialect.c++11" valueType="enumerated"/> | ||
| <option id="gnu.cpp.compiler.option.warnings.extrawarn.951690506" name="Extra warnings (-Wextra)" superClass="gnu.cpp.compiler.option.warnings.extrawarn" useByScannerDiscovery="false" value="true" valueType="boolean"/> | ||
| <option id="gnu.cpp.compiler.option.warnings.wconversion.2106545867" name="Implicit conversion warnings (-Wconversion)" superClass="gnu.cpp.compiler.option.warnings.wconversion" useByScannerDiscovery="false" value="true" valueType="boolean"/> | ||
| <inputType id="cdt.managedbuild.tool.gnu.cpp.compiler.input.268871018" superClass="cdt.managedbuild.tool.gnu.cpp.compiler.input"/> | ||
| </tool> | ||
| <tool command="gcc" commandLinePattern="${COMMAND} ${FLAGS} ${OUTPUT_FLAG} ${OUTPUT_PREFIX}${OUTPUT} ${INPUTS}" errorParsers="org.eclipse.cdt.core.GCCErrorParser" id="cdt.managedbuild.tool.gnu.c.compiler.base.1697165361" name="GCC C Compiler" superClass="cdt.managedbuild.tool.gnu.c.compiler.base"> | ||
| <option id="gnu.c.compiler.option.include.paths.23492965" name="Include paths (-I)" superClass="gnu.c.compiler.option.include.paths" useByScannerDiscovery="false" valueType="includePath"> | ||
| <listOptionValue builtIn="false" value=""${workspace_loc:/cleed++}""/> | ||
| </option> | ||
| <inputType id="cdt.managedbuild.tool.gnu.c.compiler.input.15879203" superClass="cdt.managedbuild.tool.gnu.c.compiler.input"/> | ||
| </tool> | ||
| <tool command="g++" commandLinePattern="${COMMAND} ${FLAGS} ${OUTPUT_FLAG} ${OUTPUT_PREFIX}${OUTPUT} ${INPUTS}" errorParsers="org.eclipse.cdt.core.GLDErrorParser" id="cdt.managedbuild.tool.gnu.cpp.linker.mingw.base.274172594" name="MinGW C++ Linker" superClass="cdt.managedbuild.tool.gnu.cpp.linker.mingw.base"> | ||
| <option defaultValue="true" id="gnu.cpp.link.option.shared.929989847" name="Shared (-shared)" superClass="gnu.cpp.link.option.shared" valueType="boolean"/> | ||
| <option id="gnu.cpp.link.option.paths.1793290489" name="Library search path (-L)" superClass="gnu.cpp.link.option.paths" valueType="libPaths"/> | ||
| <option id="gnu.cpp.link.option.libs.630139133" name="Libraries (-l)" superClass="gnu.cpp.link.option.libs" valueType="libs"/> | ||
| <inputType id="cdt.managedbuild.tool.gnu.cpp.linker.input.1434789074" superClass="cdt.managedbuild.tool.gnu.cpp.linker.input"> | ||
| <additionalInput kind="additionalinputdependency" paths="$(USER_OBJS)"/> | ||
| <additionalInput kind="additionalinput" paths="$(LIBS)"/> | ||
| </inputType> | ||
| </tool> | ||
| <tool command="as" commandLinePattern="${COMMAND} ${FLAGS} ${OUTPUT_FLAG} ${OUTPUT_PREFIX}${OUTPUT} ${INPUTS}" errorParsers="org.eclipse.cdt.core.GASErrorParser" id="cdt.managedbuild.tool.gnu.assembler.mingw.base.522384027" name="GCC Assembler" superClass="cdt.managedbuild.tool.gnu.assembler.mingw.base"> | ||
| <option id="gnu.both.asm.option.include.paths.73857899" name="Include paths (-I)" superClass="gnu.both.asm.option.include.paths" valueType="includePath"> | ||
| <listOptionValue builtIn="false" value=""${workspace_loc:/cleed++}""/> | ||
| <listOptionValue builtIn="false" value=""${workspace_loc:/CLEED/src/include}""/> | ||
| <listOptionValue builtIn="false" value=""${workspace_loc:/c++}""/> | ||
| </option> | ||
| <inputType id="cdt.managedbuild.tool.gnu.assembler.input.912398302" superClass="cdt.managedbuild.tool.gnu.assembler.input"/> | ||
| </tool> | ||
| <tool id="cdt.managedbuild.tool.gnu.c.linker.mingw.base.240056554" name="MinGW C Linker" superClass="cdt.managedbuild.tool.gnu.c.linker.mingw.base"> | ||
| <option defaultValue="true" id="gnu.c.link.option.shared.1205491668" name="Shared (-shared)" superClass="gnu.c.link.option.shared" valueType="boolean"/> | ||
| </tool> | ||
| <tool id="cdt.managedbuild.tool.gnu.archiver.mingw.base.211814820" name="GCC Archiver" superClass="cdt.managedbuild.tool.gnu.archiver.mingw.base"/> | ||
| </toolChain> | ||
| </folderInfo> | ||
| <sourceEntries> | ||
| <entry flags="VALUE_WORKSPACE_PATH" kind="sourcePath" name=""/> | ||
| </sourceEntries> | ||
| </configuration> | ||
| </storageModule> | ||
| <storageModule moduleId="org.eclipse.cdt.core.externalSettings"/> | ||
| </cconfiguration> | ||
| </storageModule> | ||
| <storageModule moduleId="cdtBuildSystem" version="4.0.0"> | ||
| <project id="c++.null.635482705" name="c++"/> | ||
| </storageModule> | ||
| <storageModule moduleId="org.eclipse.cdt.core.LanguageSettingsProviders"/> | ||
| <storageModule moduleId="refreshScope" versionNumber="2"> | ||
| <configuration configurationName="Debug.lib.MinGW"/> | ||
| <configuration configurationName="Default"> | ||
| <resource resourceType="PROJECT" workspacePath="/c++"/> | ||
| </configuration> | ||
| </storageModule> | ||
| <storageModule moduleId="org.eclipse.cdt.internal.ui.text.commentOwnerProjectMappings"/> | ||
| <storageModule moduleId="scannerConfiguration"> | ||
| <autodiscovery enabled="true" problemReportingEnabled="true" selectedProfileId=""/> | ||
| <scannerConfigBuildInfo instanceId="cdt.managedbuild.toolchain.gnu.mingw.base.868482903;cdt.managedbuild.toolchain.gnu.mingw.base.868482903.474468868;cdt.managedbuild.tool.gnu.c.compiler.mingw.base.1828847357;cdt.managedbuild.tool.gnu.c.compiler.input.782950281"> | ||
| <autodiscovery enabled="true" problemReportingEnabled="true" selectedProfileId=""/> | ||
| </scannerConfigBuildInfo> | ||
| <scannerConfigBuildInfo instanceId="cdt.managedbuild.toolchain.gnu.mingw.base.868482903;cdt.managedbuild.toolchain.gnu.mingw.base.868482903.474468868;cdt.managedbuild.tool.gnu.cpp.compiler.mingw.base.101507346;cdt.managedbuild.tool.gnu.cpp.compiler.input.268871018"> | ||
| <autodiscovery enabled="true" problemReportingEnabled="true" selectedProfileId=""/> | ||
| </scannerConfigBuildInfo> | ||
| <scannerConfigBuildInfo instanceId="cdt.managedbuild.toolchain.gnu.mingw.base.868482903;cdt.managedbuild.toolchain.gnu.mingw.base.868482903.474468868;cdt.managedbuild.tool.gnu.c.compiler.base.1697165361;cdt.managedbuild.tool.gnu.c.compiler.input.15879203"> | ||
| <autodiscovery enabled="true" problemReportingEnabled="true" selectedProfileId=""/> | ||
| </scannerConfigBuildInfo> | ||
| <scannerConfigBuildInfo instanceId="cdt.managedbuild.toolchain.gnu.mingw.base.1114281100;cdt.managedbuild.toolchain.gnu.mingw.base.1114281100.240320506;cdt.managedbuild.tool.gnu.cpp.compiler.mingw.base.1715693281;cdt.managedbuild.tool.gnu.cpp.compiler.input.665085222"> | ||
| <autodiscovery enabled="true" problemReportingEnabled="true" selectedProfileId=""/> | ||
| </scannerConfigBuildInfo> | ||
| <scannerConfigBuildInfo instanceId="cdt.managedbuild.toolchain.gnu.mingw.base.1114281100;cdt.managedbuild.toolchain.gnu.mingw.base.1114281100.240320506;cdt.managedbuild.tool.gnu.c.compiler.mingw.base.63988771;cdt.managedbuild.tool.gnu.c.compiler.input.1285190638"> | ||
| <autodiscovery enabled="true" problemReportingEnabled="true" selectedProfileId=""/> | ||
| </scannerConfigBuildInfo> | ||
| </storageModule> | ||
| </cproject> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider excluding IDE-specific metadata from version control.
Eclipse .cproject files contain auto-generated numeric IDs (e.g., 868482903, 474468868) and workspace-relative paths that may not be portable across different developer setups. These files often cause merge conflicts and noise in diffs.
Consider adding these to .gitignore and documenting the required Eclipse project setup separately, or ensure this is intentionally tracked per the follow-up noted in #59 regarding qmake/Eclipse metadata retention.
🤖 Prompt for AI Agents
In @src/cleed++/.cproject around lines 1 - 106, The checked-in Eclipse .cproject
contains IDE-specific, non-portable metadata (auto-generated numeric IDs like
"868482903", "474468868" and workspace paths under entries in the storageModule
elements) that causes noisy diffs and merge conflicts; remove this file from
version control (git rm --cached src/cleed++/.cproject), add
src/cleed++/.cproject to .gitignore, and instead add a short README or docs
entry (referencing the project name C++/cleed++ and the storageModule/toolChain
settings shown) that documents the required Eclipse/qmake project setup and any
retained metadata policy per issue #59; if you must keep a template, replace
numeric IDs and workspace-specific paths with placeholders before committing.
| /********************************************************************* | ||
| * RoIRFactor.h | ||
| * | ||
| * Copyright (C) 2013-2015 Liam Deacon <liam.m.deacon@gmail.com> | ||
| * | ||
| * Licensed under GNU General Public License 3.0 or later. | ||
| * Some rights reserved. See COPYING, AUTHORS. | ||
| * | ||
| * @license GPL-3.0+ <http://spdx.org/licenses/GPL-3.0+> | ||
| * | ||
| * Changes: | ||
| * LD/03.01.2015 - creation | ||
| *********************************************************************/ | ||
|
|
||
| /*! | ||
| * \file Rfactor.h |
There was a problem hiding this comment.
Filename inconsistencies in header comments.
The header comments reference incorrect filenames:
- Line 2 says "RoIRFactor.h" but the actual filename is "AoIRFactor.h"
- Line 16 says "\file Rfactor.h" but should be "AoIRFactor.h"
These inconsistencies will confuse developers and break documentation generation tools like Doxygen.
📝 Proposed fix
/*********************************************************************
- * RoIRFactor.h
+ * AoIRFactor.h
*
* Copyright (C) 2013-2015 Liam Deacon <liam.m.deacon@gmail.com>
*
* Licensed under GNU General Public License 3.0 or later.
* Some rights reserved. See COPYING, AUTHORS.
*
* @license GPL-3.0+ <http://spdx.org/licenses/GPL-3.0+>
*
* Changes:
* LD/03.01.2015 - creation
*********************************************************************/
/*!
- * \file Rfactor.h
+ * \file AoIRFactor.h
* \author Liam Deacon
*
* Angle of Incidence wrapper for RFactor class.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /********************************************************************* | |
| * RoIRFactor.h | |
| * | |
| * Copyright (C) 2013-2015 Liam Deacon <liam.m.deacon@gmail.com> | |
| * | |
| * Licensed under GNU General Public License 3.0 or later. | |
| * Some rights reserved. See COPYING, AUTHORS. | |
| * | |
| * @license GPL-3.0+ <http://spdx.org/licenses/GPL-3.0+> | |
| * | |
| * Changes: | |
| * LD/03.01.2015 - creation | |
| *********************************************************************/ | |
| /*! | |
| * \file Rfactor.h | |
| /********************************************************************* | |
| * AoIRFactor.h | |
| * | |
| * Copyright (C) 2013-2015 Liam Deacon <liam.m.deacon@gmail.com> | |
| * | |
| * Licensed under GNU General Public License 3.0 or later. | |
| * Some rights reserved. See COPYING, AUTHORS. | |
| * | |
| * @license GPL-3.0+ <http://spdx.org/licenses/GPL-3.0+> | |
| * | |
| * Changes: | |
| * LD/03.01.2015 - creation | |
| *********************************************************************/ | |
| /*! | |
| * \file AoIRFactor.h |
🤖 Prompt for AI Agents
In @src/cleed++/AoIRFactor.h around lines 1 - 16, Update the header comment to
use the correct filename "AoIRFactor.h": change the top comment title currently
reading "RoIRFactor.h" to "AoIRFactor.h" and update the Doxygen tag from "\file
Rfactor.h" to "\file AoIRFactor.h" so comments and documentation match the
actual header name (look for these strings in the comment block at the top of
AoIRFactor.h).
|
|
||
| #ifdef __cplusplus | ||
|
|
||
| #include <RFactor/RFactor.hh> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Verify the RFactor.hh header exists
fd -t f "RFactor.hh" .Repository: Liam-Deacon/CLEED
Length of output: 93
🏁 Script executed:
# Search for other RFactor includes in the codebase
rg "RFactor" --type cpp --type hRepository: Liam-Deacon/CLEED
Length of output: 6759
🏁 Script executed:
# Check if RFactor is in src or external, look at directory structure
find . -name "*RFactor*" -type d 2>/dev/null | head -20Repository: Liam-Deacon/CLEED
Length of output: 82
Include path is correct and consistent with codebase pattern.
The include <RFactor/RFactor.hh> is valid—the header exists at src/cleed++/RFactor/RFactor.hh and this angle bracket style is applied consistently across the codebase (RFactor.cc, RFactorArgs.cc, Search.hh, etc.). This approach requires the build system to add src/cleed++ to the include search path, which appears to be already configured.
🤖 Prompt for AI Agents
In @src/cleed++/AoIRFactor.h at line 28, The include line in AoIRFactor.h using
<RFactor/RFactor.hh> is correct and should be left as-is; ensure the build
system continues to add src/cleed++ to the compiler include path (so the
compiler can resolve the RFactor/RFactor.hh header referenced by the
AoIRFactor.h include).
|
|
||
| #endif /* __cplusplus */ | ||
|
|
||
| #endif /* AOIRFACTOR_H |
There was a problem hiding this comment.
CRITICAL: Unterminated comment breaks compilation.
The include guard closing comment on line 37 is missing the closing */, causing a compilation error. This will prevent any file that includes this header from compiling.
🐛 Proposed fix
-#endif /* AOIRFACTOR_H
+#endif /* AOIRFACTOR_H */Based on static analysis hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #endif /* AOIRFACTOR_H | |
| #endif /* AOIRFACTOR_H */ |
🧰 Tools
🪛 Clang (14.0.6)
[error] 37-37: unterminated /* comment
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In @src/cleed++/AoIRFactor.h at line 37, The include guard closing comment for
AOIRFACTOR_H is unterminated; update the endif line that currently reads '#endif
/* AOIRFACTOR_H' to terminate the block comment by adding '*/' so it becomes
'#endif /* AOIRFACTOR_H */' (ensure the change is made in the AoIRFactor.h
header where the #endif for AOIRFACTOR_H is declared).
| #ifndef __BulkModel_hh__ | ||
| #define __BulkModel_hh__ | ||
|
|
||
| #ifdef __cplusplus /* use C linkage if this is a C++ compiler */ | ||
|
|
||
| #include <Model.hh> | ||
| using namespace std; | ||
|
|
There was a problem hiding this comment.
Fix reserved include-guard + header include style; drop using namespace std; from header.
__BulkModel_hh__is a reserved identifier (leading__).<Model.hh>suggests a system header; for a project header it should typically be quoted.using namespace std;in a public header leaks into all includers.
Proposed patch
-#ifndef __BulkModel_hh__
-#define __BulkModel_hh__
+#ifndef CLEED_BULKMODEL_HH
+#define CLEED_BULKMODEL_HH
#ifdef __cplusplus /* use C linkage if this is a C++ compiler */
-#include <Model.hh>
-using namespace std;
+#include "Model.hh"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifndef __BulkModel_hh__ | |
| #define __BulkModel_hh__ | |
| #ifdef __cplusplus /* use C linkage if this is a C++ compiler */ | |
| #include <Model.hh> | |
| using namespace std; | |
| #ifndef CLEED_BULKMODEL_HH | |
| #define CLEED_BULKMODEL_HH | |
| #ifdef __cplusplus /* use C linkage if this is a C++ compiler */ | |
| #include "Model.hh" | |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 19-19: declaration uses identifier 'BulkModel_hh', which is a reserved identifier
(bugprone-reserved-identifier)
[error] 23-23: 'Model.hh' file not found with include; use "quotes" instead
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In @src/cleed++/BulkModel.hh around lines 18 - 25, The header uses a reserved
include-guard and leaks std namespace while treating a project header like a
system header; replace the guard symbol __BulkModel_hh__ with a project-local
macro (e.g., BULKMODEL_HH or CLEEDPP_BULKMODEL_HH), change the include from
<Model.hh> to "Model.hh" so it’s treated as a local header, and remove the line
using namespace std; from the header so the namespace does not leak to includers
(update the #ifndef/#define/#endif identifiers to match the new guard).
| /* | ||
| * Divide rfac_sum by the normalizing factor (exp_y_sum + the_y_sum) | ||
| * and return result | ||
| */ | ||
| rfac_sum /= (exp_y_sum + the_y_sum); | ||
|
|
||
| return (rfac_sum); |
There was a problem hiding this comment.
Add validation to prevent division by zero.
If the input energy list has fewer than 2 valid entries (terminated early by F_END_OF_LIST), the loop at lines 54-75 won't execute, leaving both exp_y_sum and the_y_sum at zero. Line 86 then performs division by zero, causing undefined behavior.
🛡️ Proposed fix
+ /*
+ * Validate that we have valid data before normalization
+ */
+ if ((exp_y_sum + the_y_sum) <= 0.) {
+ #ifdef ERROR
+ ERROR_MSG("(rfac_rp): insufficient data for R-factor calculation\n");
+ #endif
+ return (real)F_FAIL;
+ }
+
/*
* Divide rfac_sum by the normalizing factor (exp_y_sum + the_y_sum)
* and return result
*/
rfac_sum /= (exp_y_sum + the_y_sum);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* | |
| * Divide rfac_sum by the normalizing factor (exp_y_sum + the_y_sum) | |
| * and return result | |
| */ | |
| rfac_sum /= (exp_y_sum + the_y_sum); | |
| return (rfac_sum); | |
| /* | |
| * Validate that we have valid data before normalization | |
| */ | |
| if ((exp_y_sum + the_y_sum) <= 0.) { | |
| #ifdef ERROR | |
| ERROR_MSG("(rfac_rp): insufficient data for R-factor calculation\n"); | |
| #endif | |
| return (real)F_FAIL; | |
| } | |
| /* | |
| * Divide rfac_sum by the normalizing factor (exp_y_sum + the_y_sum) | |
| * and return result | |
| */ | |
| rfac_sum /= (exp_y_sum + the_y_sum); | |
| return (rfac_sum); |
🤖 Prompt for AI Agents
In @src/rfac/rfrp.c around lines 82 - 88, Before dividing rfac_sum by (exp_y_sum
+ the_y_sum), add a validation that the denominator (exp_y_sum + the_y_sum) is
not zero; if it is zero (e.g., due to early termination by F_END_OF_LIST leaving
fewer than two valid entries) avoid the division and return a safe value (for
example set rfac_sum = 0.0 and return it) or otherwise handle the error path
consistently with surrounding code. Place this check immediately after the loop
that accumulates exp_y_sum and the_y_sum and before the line that currently does
rfac_sum /= (exp_y_sum + the_y_sum); referencing the symbols rfac_sum,
exp_y_sum, the_y_sum and F_END_OF_LIST to locate where to add the guard.
| for (i = 0; i< iv->n_eng-1; i++) | ||
| { | ||
| for (j = i+1; j< iv->n_eng; j++) | ||
| { | ||
| if(iv->data[j].energy < iv->data[i].energy) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor style: add spaces around operators and after keywords.
For consistency with common C style guides, consider adding spaces after < operators and if keywords (lines 56, 58, 60).
💅 Style refinement
- for (i = 0; i< iv->n_eng-1; i++)
+ for (i = 0; i < iv->n_eng - 1; i++)
{
- for (j = i+1; j< iv->n_eng; j++)
+ for (j = i + 1; j < iv->n_eng; j++)
{
- if(iv->data[j].energy < iv->data[i].energy)
+ if (iv->data[j].energy < iv->data[i].energy)🤖 Prompt for AI Agents
In @src/rfac/rfsort.c around lines 56 - 60, The for-loops and if statement in
rfsort.c lack spaces around operators and after keywords; update the loops and
condition (the loops iterating over iv->n_eng and the if checking
iv->data[j].energy < iv->data[i].energy) to use consistent spacing (e.g., add
spaces after keywords like for and if, spaces around < and <, around < and <
comparisons) so the code reads with standard C spacing conventions.
| for (i = 0; i< iv->n_eng-1; i++) | ||
| { | ||
| for (j = i+1; j< iv->n_eng; j++) | ||
| { | ||
| if(iv->data[j].energy < iv->data[i].energy) | ||
| { | ||
| /* exchange energy values */ | ||
| f_aux = iv->data[i].energy; | ||
| iv->data[i].energy = iv->data[j].energy; | ||
| iv->data[j].energy = f_aux; | ||
|
|
||
| /* exchange intensities */ | ||
| f_aux = iv->data[i].intens; | ||
| iv->data[i].intens = iv->data[j].intens; | ||
| iv->data[j].intens = f_aux; | ||
| } | ||
|
|
||
| } /* for j */ | ||
|
|
||
| } /* for i */ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Selection sort has O(n²) complexity.
The implementation uses a selection sort algorithm which has quadratic time complexity. For large IV curves (n > 1000 points), this may be slow. Consider whether performance optimization is needed based on typical dataset sizes.
If datasets are typically small (< 100 points), the current implementation is fine. For larger datasets, consider using qsort() from <stdlib.h> or a more efficient algorithm.
🤖 Prompt for AI Agents
In @src/rfac/rfsort.c around lines 56 - 75, The nested loops in rfsort.c perform
a quadratic selection sort over iv->data (using iv->n_eng and fields
energy/intens) which is slow for large n; replace this manual O(n²) sort with a
call to qsort: add <stdlib.h>, implement a comparator that compares elements'
energy (e.g., comparing ((const typeof(iv->data[0]) *)a)->energy and ->energy of
b) to produce ascending order, and call qsort(iv->data, iv->n_eng,
sizeof(iv->data[0]), comparator) so the entire struct (both energy and intens)
is reordered correctly and performance improves for large IV curves.
| iv->last_eng = iv->data[iv->n_eng - 1].energy; | ||
| iv->sort = true; | ||
|
|
||
| return (0); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Return value inconsistent with documentation.
The function returns 0 but the documentation (lines 39-42) specifies return values RFAC_SUCCESS and RFAC_FAILURE. For consistency and clarity, use the documented constant.
♻️ Proposed fix
iv->first_eng = iv->data[0].energy;
iv->last_eng = iv->data[iv->n_eng - 1].energy;
iv->sort = true;
- return (0);
+ return (RFAC_SUCCESS);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return (0); | |
| iv->first_eng = iv->data[0].energy; | |
| iv->last_eng = iv->data[iv->n_eng - 1].energy; | |
| iv->sort = true; | |
| return (RFAC_SUCCESS); | |
| } |
🤖 Prompt for AI Agents
In @src/rfac/rfsort.c at line 91, The return at the end of the function in
src/rfac/rfsort.c currently returns the literal 0; change it to use the
documented constants by returning RFAC_SUCCESS on success (or RFAC_FAILURE on
error paths) so the function's return value matches the documentation; locate
the function containing that final "return (0);" and replace the literal with
RFAC_SUCCESS (and ensure any error branches return RFAC_FAILURE).
| /*! | ||
| * Performs the first stage of a cubic spline. | ||
| * | ||
| * \param[in,out] *list list of energy/intensity values to be | ||
| * interpolated by cubic spline. | ||
| * The function will generate the structure elements deriv2, which will | ||
| * be used for interpolation in function rfac_splint() . | ||
| * | ||
| * For a description see standard references on cubic spline interpolation. | ||
| * The boundary conditions are set for a natural spline | ||
| * (zero second derivative on both boundaries). | ||
| * | ||
| */ |
There was a problem hiding this comment.
Fix Doxygen param name to unblock Docs CI (warnings-as-errors).
Docs job is failing because \param ... list doesn’t exist; the parameter is iv.
Proposed patch
- * \param[in,out] *list list of energy/intensity values to be
- * interpolated by cubic spline.
+ * \param[in,out] iv IV curve (energy/intensity pairs) to be prepared for
+ * spline interpolation.🧰 Tools
🪛 GitHub Actions: Docs (Sphinx)
[error] 26-26: Doxygen: argument 'list' of @param is not found in the parameter list of rfac_iv_spline(rfac_iv *iv) (warnings treated as errors).
🤖 Prompt for AI Agents
In @src/rfac/rfspline.c around lines 25 - 37, The Doxygen parameter name is
wrong: update the comment for function rfac_spline to use the actual parameter
name iv instead of "list"; change the param tag to "\param[in,out] iv" and keep
the description ("list of energy/intensity values to be interpolated...") so the
docs generator finds the real parameter and CI warnings go away.
| add_library(cleedCore SHARED ${cleed_core_SRCS}) | ||
| add_library(cleedCoreStatic STATIC ${cleed_core_SRCS}) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if export macros are already defined in headers
find src/include -type f -name "*.h" -o -name "*.hpp" | head -20Repository: Liam-Deacon/CLEED
Length of output: 518
🏁 Script executed:
#!/bin/bash
# Check if export macros are already defined in headers
rg -n "dllexport|dllimport|CLEED.*EXPORT" src/Repository: Liam-Deacon/CLEED
Length of output: 3378
🏁 Script executed:
#!/bin/bash
# Check the CMakeLists.txt content to see what's actually there
cat -n src/cleed_core/CMakeLists.txtRepository: Liam-Deacon/CLEED
Length of output: 1247
🏁 Script executed:
#!/bin/bash
# Check for public functions/symbols in cleed_core related headers
rg -A 2 "^(HPDF_)?[A-Za-z_].*\(" src/include/cleed_mode.h src/include/leed.h src/include/leed_func.h 2>/dev/null | head -50Repository: Liam-Deacon/CLEED
Length of output: 3549
🏁 Script executed:
#!/bin/bash
# Check if there's a cleed_core.h header file
find src/include -name "*cleed*core*" -type fRepository: Liam-Deacon/CLEED
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Check what symbols are actually used from cleedCore library in the codebase
rg "cleedCore|cleed_core" --type cmake src/ | head -20Repository: Liam-Deacon/CLEED
Length of output: 1272
🏁 Script executed:
#!/bin/bash
# Check the content of cleed_core.h to see if export macros are used
cat -n src/include/cleed_core.hRepository: Liam-Deacon/CLEED
Length of output: 2182
Consider adding export macros for Windows DLL symbol visibility.
The public API functions in cleed_core.h are declared without export macros. While this works on Windows (all symbols are exported by default), using CMake's generate_export_header() to create explicit __declspec(dllexport) / __declspec(dllimport) macros is a best practice that would:
- Make the DLL smaller by hiding internal symbols
- Provide explicit control over the public API boundary
- Improve consistency with other libraries in the codebase that already use export macros (e.g., libharu, zlib)
Since the CMakeLists.txt already includes Windows-specific build logic, adding export header generation would complete the Windows library support:
include(GenerateExportHeader)
generate_export_header(cleedCore
EXPORT_FILE_NAME ${CMAKE_BINARY_DIR}/cleed_core_export.h
BASE_NAME CLEED_CORE)Then decorate public functions in cleed_core.h with CLEED_CORE_EXPORT.
🤖 Prompt for AI Agents
In @src/cleed_core/CMakeLists.txt around lines 8 - 9, Add explicit Windows
export macros: modify the cleedCore target to call CMake's GenerateExportHeader
(use include(GenerateExportHeader) and generate_export_header(cleedCore
EXPORT_FILE_NAME ${CMAKE_BINARY_DIR}/cleed_core_export.h BASE_NAME CLEED_CORE))
and then update the public API declarations in cleed_core.h to use the generated
CLEED_CORE_EXPORT macro for exported functions/types; ensure the generated
header is included where cleed_core.h is compiled and when consuming the shared
library so symbols are correctly decorated for dllexport/dllimport on Windows.
| #ifndef __ATOM_HH__ | ||
| #define __ATOM_HH__ |
There was a problem hiding this comment.
Avoid reserved identifiers for include guards.
Identifiers beginning with double underscores (__ATOM_HH__) are reserved for the implementation. Use a project-specific prefix instead.
Proposed fix
-#ifndef __ATOM_HH__
-#define __ATOM_HH__
+#ifndef CLEED_CORE_ATOM_HH
+#define CLEED_CORE_ATOM_HH
...
-#endif /* __ATOM_HH__ */
+#endif /* CLEED_CORE_ATOM_HH */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifndef __ATOM_HH__ | |
| #define __ATOM_HH__ | |
| #ifndef CLEED_CORE_ATOM_HH | |
| #define CLEED_CORE_ATOM_HH |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 19-19: declaration uses identifier 'ATOM_HH', which is a reserved identifier
(bugprone-reserved-identifier)
🤖 Prompt for AI Agents
In @src/cleed++/Core/Atom.hh around lines 18 - 19, The include guard uses a
reserved identifier __ATOM_HH__; change it to a project-specific macro (e.g.,
CLEEDPP_ATOM_HH or CLEED_ATOM_HH) and update every occurrence consistently:
replace __ATOM_HH__ in the #ifndef and #define at the top of Atom.hh and adjust
the matching #endif comment if present so the macro name matches exactly.
|
|
||
| #include <Core/BaseElement.hh> | ||
|
|
||
| using namespace std; |
There was a problem hiding this comment.
Remove using namespace std; from header file.
This pollutes the global namespace for all files that include this header, potentially causing name collisions. Use explicit std:: qualification instead (already done for most types in this file).
Proposed fix
-using namespace std;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using namespace std; |
🤖 Prompt for AI Agents
In @src/cleed++/Core/Atom.hh at line 30, Remove the global "using namespace
std;" declaration from the Atom.hh header and replace any remaining unqualified
standard identifiers with explicit std:: qualifiers; specifically, delete the
"using namespace std;" line in Atom.hh and verify identifiers used in class Atom
and related declarations (e.g., string, vector, size_t, ostream operators or
other standard types/functions) are fully prefixed with std:: to avoid leaking
std symbols into global scope.
| class Atom : public BaseElement { | ||
| public: | ||
| Atom(); | ||
| Atom(double x, double y, double z); | ||
| Atom(std::vector<double> pos); | ||
| virtual ~Atom(); | ||
|
|
||
| friend std::ostream& operator<<(std::ostream& out, const Atom& atom); | ||
|
|
||
| /* pure virtual functions */ | ||
| virtual double getXPosition() const = 0; | ||
| virtual double getYPosition() const = 0; | ||
| virtual double getZPosition() const = 0; | ||
| virtual std::vector<double> getPosition() const = 0; | ||
|
|
||
| virtual Atom& setXPosition(double x_pos) = 0; | ||
| virtual Atom& setYPosition(double y_pos) = 0; | ||
| virtual Atom& setZPosition(double z_pos) = 0; | ||
| virtual Atom& setPosition(std::vector<double> pos) = 0; | ||
|
|
||
| private: | ||
| double x; | ||
| double y; | ||
| double z; | ||
| }; |
There was a problem hiding this comment.
Design issue: Abstract class with inaccessible private data members.
Atom declares pure virtual getters/setters (lines 44-52) but also has private x, y, z members (lines 55-57). Derived classes cannot access these private members, making them useless. Additionally, the implementation in Atom.cc shows constructors calling pure virtual methods (setXPosition, etc.), which is undefined behavior.
Consider one of these approaches:
- Make
Atomconcrete by providing default implementations instead of pure virtuals - Remove private members and let derived classes manage their own storage
- Make members
protectedso derived classes can access them
Option 1: Make Atom concrete with protected members
class Atom : public BaseElement {
public:
// ...
- /* pure virtual functions */
- virtual double getXPosition() const = 0;
- virtual double getYPosition() const = 0;
- virtual double getZPosition() const = 0;
- virtual std::vector<double> getPosition() const = 0;
-
- virtual Atom& setXPosition(double x_pos) = 0;
- virtual Atom& setYPosition(double y_pos) = 0;
- virtual Atom& setZPosition(double z_pos) = 0;
- virtual Atom& setPosition(std::vector<double> pos) = 0;
+ virtual double getXPosition() const;
+ virtual double getYPosition() const;
+ virtual double getZPosition() const;
+ virtual std::vector<double> getPosition() const;
+
+ virtual Atom& setXPosition(double x_pos);
+ virtual Atom& setYPosition(double y_pos);
+ virtual Atom& setZPosition(double z_pos);
+ virtual Atom& setPosition(std::vector<double> pos);
- private:
+ protected:
double x;
double y;
double z;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Atom : public BaseElement { | |
| public: | |
| Atom(); | |
| Atom(double x, double y, double z); | |
| Atom(std::vector<double> pos); | |
| virtual ~Atom(); | |
| friend std::ostream& operator<<(std::ostream& out, const Atom& atom); | |
| /* pure virtual functions */ | |
| virtual double getXPosition() const = 0; | |
| virtual double getYPosition() const = 0; | |
| virtual double getZPosition() const = 0; | |
| virtual std::vector<double> getPosition() const = 0; | |
| virtual Atom& setXPosition(double x_pos) = 0; | |
| virtual Atom& setYPosition(double y_pos) = 0; | |
| virtual Atom& setZPosition(double z_pos) = 0; | |
| virtual Atom& setPosition(std::vector<double> pos) = 0; | |
| private: | |
| double x; | |
| double y; | |
| double z; | |
| }; | |
| class Atom : public BaseElement { | |
| public: | |
| Atom(); | |
| Atom(double x, double y, double z); | |
| Atom(std::vector<double> pos); | |
| virtual ~Atom(); | |
| friend std::ostream& operator<<(std::ostream& out, const Atom& atom); | |
| /* pure virtual functions */ | |
| virtual double getXPosition() const; | |
| virtual double getYPosition() const; | |
| virtual double getZPosition() const; | |
| virtual std::vector<double> getPosition() const; | |
| virtual Atom& setXPosition(double x_pos); | |
| virtual Atom& setYPosition(double y_pos); | |
| virtual Atom& setZPosition(double z_pos); | |
| virtual Atom& setPosition(std::vector<double> pos); | |
| protected: | |
| double x; | |
| double y; | |
| double z; | |
| }; |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 37-37: parameter name 'x' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 37-37: parameter name 'y' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 37-37: parameter name 'z' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 41-41: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 44-44: function 'getXPosition' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 44-44: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 45-45: function 'getYPosition' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 45-45: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 46-46: function 'getZPosition' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 46-46: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 47-47: function 'getPosition' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 47-47: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 49-49: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 50-50: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 51-51: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 52-52: use a trailing return type for this function
(modernize-use-trailing-return-type)
🤖 Prompt for AI Agents
In @src/cleed++/Core/Atom.hh around lines 34 - 58, The Atom class currently
declares private members x,y,z and pure-virtual accessors
(getXPosition/getYPosition/getZPosition/getPosition and
setXPosition/setYPosition/setZPosition/setPosition) which makes the members
unusable and calling those virtual setters from Atom() is undefined; fix by
changing x,y,z to protected, provide concrete default implementations for
getXPosition/getYPosition/getZPosition/getPosition and
setXPosition/setYPosition/setZPosition/setPosition in Atom (remove the =0), and
modify Atom constructors to initialize the protected members directly (do not
call virtual methods like setXPosition inside Atom::Atom).
| BaseElement::BaseElement() { | ||
|
|
||
| } | ||
|
|
||
| BaseElement::BaseElement(std::size_t num) { | ||
|
|
||
| } | ||
|
|
||
| BaseElement::BaseElement(const std::string &name) { | ||
|
|
||
| } | ||
|
|
||
| BaseElement::BaseElement(const char symbol[]) { | ||
|
|
||
| } | ||
|
|
||
| BaseElement::~BaseElement() { | ||
|
|
||
| } |
There was a problem hiding this comment.
Constructors do not initialize members or use parameters.
All constructors have empty bodies, leaving atomicNumber, name, and symbol uninitialized. The parameterized constructors accept arguments but ignore them. This leads to undefined behavior when getters are called.
Proposed fix
-BaseElement::BaseElement() {
-
+BaseElement::BaseElement() : atomicNumber(0), name(""), symbol{'\0', '\0', '\0'} {
}
-BaseElement::BaseElement(std::size_t num) {
-
+BaseElement::BaseElement(std::size_t num) : BaseElement() {
+ setAtomicNumber(num);
}
-BaseElement::BaseElement(const std::string &name) {
-
+BaseElement::BaseElement(const std::string &name) : BaseElement() {
+ setElementName(name);
}
-BaseElement::BaseElement(const char symbol[]) {
-
+BaseElement::BaseElement(const char symbol[]) : BaseElement() {
+ setElementSymbol(symbol);
}| #include <stddef.h> | ||
| #include <stdbool.h> | ||
| #include <stdio.h> | ||
| #include <malloc.h> |
There was a problem hiding this comment.
Replace non-portable <malloc.h> with <stdlib.h>.
<malloc.h> is not part of standard C and may not exist on all platforms (e.g., macOS). The <stdlib.h> header (already included at line 20) provides malloc, calloc, free, and realloc.
Proposed fix
-#include <malloc.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <malloc.h> |
🧰 Tools
🪛 Cppcheck (2.19.0)
[information] 24-24: Include file
(missingIncludeSystem)
🤖 Prompt for AI Agents
In @src/rfac/rfrdexpt.c at line 24, The file currently includes the non-portable
header "<malloc.h>"; remove that include and rely on the already-present
"<stdlib.h>" for malloc/calloc/realloc/free declarations (i.e., delete the
"#include <malloc.h>" line in rfrdexpt.c so the code uses the standard header
instead).
| if( (buffer = file2buffer(filename)) == NULL) | ||
| { | ||
| ERROR_MSG("error in function file2buffer \n"); | ||
| return(NULL); | ||
| } |
There was a problem hiding this comment.
Memory leak: iv not freed on file2buffer failure.
If file2buffer returns NULL, the function returns without freeing the iv structure allocated by rfac_iv_init() at line 53.
Proposed fix
if( (buffer = file2buffer(filename)) == NULL)
{
ERROR_MSG("error in function file2buffer \n");
+ rfac_iv_free(iv);
return(NULL);
}🤖 Prompt for AI Agents
In @src/rfac/rfrdexpt.c around lines 65 - 69, If file2buffer() returns NULL you
currently return without freeing the iv allocated by rfac_iv_init(); fix by
freeing iv before returning on buffer failure. In the error branch where buffer
== NULL, call the corresponding iv cleanup function used in this module (e.g.,
rfac_iv_free(iv) or rfac_iv_destroy(iv) — whatever the codebase uses) and then
set iv to NULL if appropriate before returning NULL.
| lines = rfac_nclines(buffer); | ||
|
|
||
| CONTROL_MSG(CONTROL_X, "file content:\n%s\n", buffer); | ||
| CONTROL_MSG(CONTROL_X, "lines = %d\n", lines); |
There was a problem hiding this comment.
Use %zu format specifier for size_t.
lines is size_t, but %d is used. This causes undefined behavior on platforms where size_t differs from int.
Proposed fix
- CONTROL_MSG(CONTROL_X, "lines = %d\n", lines);
+ CONTROL_MSG(CONTROL_X, "lines = %zu\n", lines);🤖 Prompt for AI Agents
In @src/rfac/rfrdexpt.c at line 73, The CONTROL_MSG call uses "%d" for the
variable lines which is a size_t; change the format specifier to "%zu" in the
CONTROL_MSG invocation that prints lines (i.e., CONTROL_MSG(CONTROL_X, "lines =
%zu\n", lines)); ensure you do not cast lines to an incompatible type and keep
the argument as the size_t variable to avoid undefined behavior.
| /* print eV[i] I[i] */ | ||
| CONTROL_MSG(CONTROL_X, "(%s): %e %e\n", filename, | ||
| iv->data[i].energy, iv->data[i].intens); | ||
|
|
||
| if(sscanf(line_buffer, fmt_buffer, &iv->data[i].energy, &iv->data[i].intens) == 2) |
There was a problem hiding this comment.
Debug print uses uninitialized values.
The CONTROL_MSG at lines 92-93 prints iv->data[i].energy and iv->data[i].intens before they are assigned by sscanf at line 95. Move the debug print after the successful parse.
Proposed fix
if(line_buffer[0] != '#') /* no comment */
{
-
- /* print eV[i] I[i] */
- CONTROL_MSG(CONTROL_X, "(%s): %e %e\n", filename,
- iv->data[i].energy, iv->data[i].intens);
-
if(sscanf(line_buffer, fmt_buffer, &iv->data[i].energy, &iv->data[i].intens) == 2)
{
+ /* print eV[i] I[i] */
+ CONTROL_MSG(CONTROL_X, "(%s): %e %e\n", filename,
+ iv->data[i].energy, iv->data[i].intens);
+
/* search for max. intensity */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* print eV[i] I[i] */ | |
| CONTROL_MSG(CONTROL_X, "(%s): %e %e\n", filename, | |
| iv->data[i].energy, iv->data[i].intens); | |
| if(sscanf(line_buffer, fmt_buffer, &iv->data[i].energy, &iv->data[i].intens) == 2) | |
| if(sscanf(line_buffer, fmt_buffer, &iv->data[i].energy, &iv->data[i].intens) == 2) | |
| { | |
| /* print eV[i] I[i] */ | |
| CONTROL_MSG(CONTROL_X, "(%s): %e %e\n", filename, | |
| iv->data[i].energy, iv->data[i].intens); |
🤖 Prompt for AI Agents
In @src/rfac/rfrdexpt.c around lines 91 - 95, The debug print is using
iv->data[i].energy and iv->data[i].intens before they are populated by sscanf;
change the flow so sscanf(line_buffer, fmt_buffer, &iv->data[i].energy,
&iv->data[i].intens) is called first and then, only if it returns 2, call
CONTROL_MSG(CONTROL_X, "(%s): %e %e\n", filename, iv->data[i].energy,
iv->data[i].intens). Locate the sscanf and CONTROL_MSG calls in rfrdexpt.c and
move the CONTROL_MSG into the successful-parse branch to avoid printing
uninitialized values.
| if (!iv) | ||
| goto error; | ||
| if (!iv->data) | ||
| goto error; |
There was a problem hiding this comment.
Remove unreachable null checks.
These checks for iv and iv->data are placed after iv has already been dereferenced extensively (lines 75-143). If iv were NULL, the code would have already crashed. Either move these checks earlier or remove them.
Proposed fix - remove dead code
- if (!iv)
- goto error;
- if (!iv->data)
- goto error;
-
/* write available information to structure iv_cur. */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!iv) | |
| goto error; | |
| if (!iv->data) | |
| goto error; | |
| /* write available information to structure iv_cur. */ |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 144-144: statement should be inside braces
(readability-braces-around-statements)
[warning] 146-146: statement should be inside braces
(readability-braces-around-statements)
🤖 Prompt for AI Agents
In @src/rfac/rfrdexpt.c around lines 144 - 147, The two null-check blocks "if
(!iv) goto error;" and "if (!iv->data) goto error;" in rfrdexpt.c are
unreachable because `iv` is already dereferenced in the code between lines
75–143; remove these dead checks (delete the two if statements) or, if the
intent was to guard before any dereference, move them up to run immediately
after `iv` is assigned/returned and before any use of `iv` or `iv->data`; update
any related error handling accordingly so no redundant checks remain.
Doxygen was erroneously interpreting multiple \retval statements with 'if' clauses as having 'multiple documentation sections'.
| #ifndef __BASEELEMENT_HH__ | ||
| #define __BASEELEMENT_HH__ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid reserved identifiers in header guards.
Identifiers with double leading underscores are reserved. Use a project-prefixed form.
Suggested fix
-#ifndef __BASEELEMENT_HH__
-#define __BASEELEMENT_HH__
+#ifndef CLEED_BASEELEMENT_HH
+#define CLEED_BASEELEMENT_HH📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifndef __BASEELEMENT_HH__ | |
| #define __BASEELEMENT_HH__ | |
| #ifndef CLEED_BASEELEMENT_HH | |
| #define CLEED_BASEELEMENT_HH |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 19-19: declaration uses identifier 'BASEELEMENT_HH', which is a reserved identifier
(bugprone-reserved-identifier)
🤖 Prompt for AI Agents
In @src/cleed++/Core/BaseElement.hh around lines 18 - 19, The header guard
currently uses a reserved identifier __BASEELEMENT_HH__; replace it with a
project-prefixed macro (for example CLEEDPP_BASEELEMENT_HH or
CLEEDPP_CORE_BASEELEMENT_HH) and update the matching #ifndef and #define (and
any trailing #endif comment) to use that new macro name so the guard is not a
double-underscore reserved identifier and stays consistent within
BaseElement.hh.
| using std::size_t; | ||
| using std::string; |
There was a problem hiding this comment.
Avoid using directives at namespace scope in headers.
These using declarations pollute the global namespace for all files that include this header. Remove them and use fully qualified names (std::size_t, std::string) in the declarations.
Suggested fix
-using std::size_t;
-using std::string;The declarations already use std::size_t and std::string, so no other changes are needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using std::size_t; | |
| using std::string; |
🤖 Prompt for AI Agents
In @src/cleed++/Core/BaseElement.hh around lines 25 - 26, Remove the two
namespace-qualifying using declarations ("using std::size_t;" and "using
std::string;") from the header (BaseElement.hh) to avoid polluting the global
namespace; verify that all declarations in this file continue to use
fully-qualified std::size_t and std::string (they already do) so no other
changes are required.
| private: | ||
| std::size_t atomicNumber; | ||
| std::string name; | ||
| char symbol[2]; |
There was a problem hiding this comment.
Members are left uninitialized in constructors.
Per the implementation in BaseElement.cc, all constructors have empty bodies. This leaves atomicNumber and symbol with indeterminate values. Consider providing default member initializers.
Suggested fix
private:
- std::size_t atomicNumber;
+ std::size_t atomicNumber{0};
std::string name;
- char symbol[2];
+ char symbol[2]{'\0', '\0'};Alternatively, implement the constructor bodies in BaseElement.cc to properly initialize these members based on the constructor parameters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private: | |
| std::size_t atomicNumber; | |
| std::string name; | |
| char symbol[2]; | |
| private: | |
| std::size_t atomicNumber{0}; | |
| std::string name; | |
| char symbol[2]{'\0', '\0'}; |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 52-52: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
🤖 Prompt for AI Agents
In @src/cleed++/Core/BaseElement.hh around lines 49 - 52, The BaseElement class
members atomicNumber, name and symbol are left uninitialized because all
constructors in BaseElement.cc have empty bodies; either add default member
initializers in the class declaration (initialize atomicNumber to 0, name to
empty string, and symbol to a valid zero-terminated array) or implement the
constructors in BaseElement.cc to assign the passed-in values to atomicNumber,
name and copy/terminate symbol appropriately; update the constructors that
currently have empty bodies to set these members (e.g., the BaseElement(...)
overloads) so atomicNumber, name and symbol are always initialized.
| #ifndef __BASIS_HH__ | ||
| #define __BASIS_HH__ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid reserved identifiers in header guards.
Use a project-prefixed identifier without leading double underscores.
Suggested fix
-#ifndef __BASIS_HH__
-#define __BASIS_HH__
+#ifndef CLEED_BASIS_HH
+#define CLEED_BASIS_HH📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifndef __BASIS_HH__ | |
| #define __BASIS_HH__ | |
| #ifndef CLEED_BASIS_HH | |
| #define CLEED_BASIS_HH |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 24-24: declaration uses identifier 'BASIS_HH', which is a reserved identifier
(bugprone-reserved-identifier)
🤖 Prompt for AI Agents
In @src/cleed++/Core/Basis.hh around lines 23 - 24, The header guard in Basis.hh
uses a reserved identifier __BASIS_HH__; change it to a project-prefixed,
non-reserved macro (e.g. CLEEDPP_CORE_BASIS_HH or CLEEDPP_BASIS_H) throughout
the file, updating both the #ifndef/#define and the corresponding #endif comment
to use the new macro name so the guard consistently matches.
| Coordinate::Coordinate(const coord_t *position) | ||
| { | ||
| this->pos = ::coord_init(); | ||
| ::coord_copy(this->pos, position); | ||
| } |
There was a problem hiding this comment.
Missing null check - potential null pointer dereference.
If position is null, coord_copy will likely dereference it, causing undefined behavior.
Proposed fix
Coordinate::Coordinate(const coord_t *position)
{
this->pos = ::coord_init();
- ::coord_copy(this->pos, position);
+ if (position) {
+ ::coord_copy(this->pos, position);
+ }
}🧰 Tools
🪛 Clang (14.0.6)
[warning] 39-39: constructor does not initialize these fields: pos
(cppcoreguidelines-pro-type-member-init)
🤖 Prompt for AI Agents
In @src/cleed++/Core/Coordinate.cc around lines 39 - 43, The constructor
Coordinate::Coordinate(const coord_t *position) dereferences position via
::coord_copy without checking for nullptr; add a null-check for the incoming
position pointer before calling ::coord_copy (e.g., after calling ::coord_init()
assign this->pos, then if position is non-null call ::coord_copy(this->pos,
position) else initialize this->pos to a safe default or handle the
error/throw), ensuring pos is always valid and avoiding a null pointer
dereference.
| namespace cleed { | ||
|
|
||
|
|
||
| class RFactorArgs : public rfac_args { |
There was a problem hiding this comment.
Redundant inheritance and composition: class both inherits from rfac_args AND has a c_args member.
RFactorArgs publicly inherits from rfac_args (line 43), which means it already IS-A rfac_args. Having an additional rfac_args c_args member (line 88) is redundant and confusing. This creates two rfac_args instances per object.
Choose one approach:
- Composition (HAS-A): Remove inheritance, keep the
c_argsmember - Inheritance (IS-A): Remove the
c_argsmember, access inherited fields directly
Proposed fix (composition approach)
-class RFactorArgs : public rfac_args {
+class RFactorArgs {
public:
// ... declarations unchanged ...
private:
rfac_args c_args;
};Also applies to: 87-88
🤖 Prompt for AI Agents
In @src/cleed++/RFactor/RFactorArgs.hh at line 43, RFactorArgs currently both
inherits from rfac_args and contains a rfac_args member c_args, creating
duplicate state; pick one model and remove the other — I recommend keeping
inheritance (IS-A) to preserve existing interface: delete the rfac_args c_args
member, remove any uses of c_args in constructors and methods and replace them
with direct access to inherited fields/methods (update any initialization lists
or copies that referenced c_args to use the base class members instead), and
remove redundant copies/assignments; alternatively, if you prefer composition,
remove the ": public rfac_args" base class, keep c_args, and forward or expose
needed fields via accessors — ensure constructors, assignment, and all
references are updated to the chosen design so only a single rfac_args instance
exists per RFactorArgs.
| void SearchAtom::setPosition(std::vector<double> pos) { | ||
| this->setXPosition(pos.at(0)); | ||
| this->setYPosition(pos.at(1)); | ||
| this->setZPosition(pos.at(2)); | ||
| } |
There was a problem hiding this comment.
setPosition may throw if vector has fewer than 3 elements.
Using pos.at(0), pos.at(1), pos.at(2) will throw std::out_of_range if the vector is too small. Consider validating size or documenting the precondition.
Proposed fix
void SearchAtom::setPosition(std::vector<double> pos) {
+ if (pos.size() < 3) {
+ throw std::invalid_argument("Position vector must have at least 3 elements");
+ }
this->setXPosition(pos.at(0));
this->setYPosition(pos.at(1));
this->setZPosition(pos.at(2));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void SearchAtom::setPosition(std::vector<double> pos) { | |
| this->setXPosition(pos.at(0)); | |
| this->setYPosition(pos.at(1)); | |
| this->setZPosition(pos.at(2)); | |
| } | |
| void SearchAtom::setPosition(std::vector<double> pos) { | |
| if (pos.size() < 3) { | |
| throw std::invalid_argument("Position vector must have at least 3 elements"); | |
| } | |
| this->setXPosition(pos.at(0)); | |
| this->setYPosition(pos.at(1)); | |
| this->setZPosition(pos.at(2)); | |
| } |
🤖 Prompt for AI Agents
In @src/cleed++/Search/SearchAtom.cc around lines 149 - 153,
SearchAtom::setPosition currently uses pos.at(0/1/2) which will throw
std::out_of_range for vectors smaller than 3; update the method to validate that
pos.size() >= 3 (or assert/document the precondition) before accessing elements
and handle the error path (return early, throw a clearer exception, or log) and
then call setXPosition/setYPosition/setZPosition only when the size check passes
so the method no longer unexpectedly throws from std::vector::at.
| #ifndef __SearchAtom_H__ | ||
| #define __SearchAtom_H__ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reserved identifier used for include guard.
Suggested fix
-#ifndef __SearchAtom_H__
-#define __SearchAtom_H__
+#ifndef CLEED_SEARCHATOM_HH
+#define CLEED_SEARCHATOM_HH📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifndef __SearchAtom_H__ | |
| #define __SearchAtom_H__ | |
| #ifndef CLEED_SEARCHATOM_HH | |
| #define CLEED_SEARCHATOM_HH |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 25-25: declaration uses identifier 'SearchAtom_H', which is a reserved identifier
(bugprone-reserved-identifier)
🤖 Prompt for AI Agents
In @src/cleed++/Search/SearchAtom.hh around lines 24 - 25, The include guard
uses a reserved identifier "__SearchAtom_H__"; change it to a non-reserved,
project-scoped macro (for example SEARCHATOM_H or CLEEDPP_SEARCH_SEARCHATOM_H)
and update both occurrences in the header (#ifndef and #define) and any matching
#endif comment to match the new macro name so the guard still works correctly;
ensure no leading underscores followed by a capital letter or double underscores
remain.
| class SearchAtom : public search_atom { | ||
| public: | ||
| SearchAtom(); | ||
| SearchAtom(const SearchAtom &other); | ||
| ~SearchAtom(); |
There was a problem hiding this comment.
Destructor must be virtual when class has virtual methods.
The class declares virtual methods (lines 52-55, 70-73) but the destructor is non-virtual. If a derived class is deleted through a SearchAtom* pointer, the derived destructor won't be called, causing undefined behavior and potential resource leaks.
Proposed fix
SearchAtom();
SearchAtom(const SearchAtom &other);
- ~SearchAtom();
+ virtual ~SearchAtom();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class SearchAtom : public search_atom { | |
| public: | |
| SearchAtom(); | |
| SearchAtom(const SearchAtom &other); | |
| ~SearchAtom(); | |
| class SearchAtom : public search_atom { | |
| public: | |
| SearchAtom(); | |
| SearchAtom(const SearchAtom &other); | |
| virtual ~SearchAtom(); |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 38-38: destructor of 'SearchAtom' is public and non-virtual
(cppcoreguidelines-virtual-class-destructor)
[note] 38-38: make it public and virtual
(clang)
🤖 Prompt for AI Agents
In @src/cleed++/Search/SearchAtom.hh around lines 38 - 42, SearchAtom declares
virtual methods but its destructor ~SearchAtom() is non-virtual; change the
declaration to a virtual destructor (e.g., virtual ~SearchAtom()) and
update/keep the corresponding definition/implementation in the .cc/.cpp file to
match so derived destructors run when deleted via a SearchAtom*; ensure the
destructor remains defined (or =default) where SearchAtom is implemented.
|
|
||
| /* setters */ | ||
| void setType(int type); | ||
| void setNumberOfEquivelentAtoms(std::size_t ref); |
There was a problem hiding this comment.
Typo in method name: "Equivelent" should be "Equivalent".
This is a public API, so fixing the typo now prevents breaking changes later.
Proposed fix
- void setNumberOfEquivelentAtoms(std::size_t ref);
+ void setNumberOfEquivalentAtoms(std::size_t ref);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void setNumberOfEquivelentAtoms(std::size_t ref); | |
| void setNumberOfEquivalentAtoms(std::size_t ref); |
🤖 Prompt for AI Agents
In @src/cleed++/Search/SearchAtom.hh at line 64, Method name
setNumberOfEquivelentAtoms contains a spelling mistake; rename it to
setNumberOfEquivalentAtoms across the public API: update the declaration in
SearchAtom.hh (replace setNumberOfEquivelentAtoms with
setNumberOfEquivalentAtoms), update the corresponding definition/implementation
(e.g., SearchAtom::setNumberOfEquivelentAtoms) and all call sites, and keep the
old symbol as a deprecated wrapper if ABI compatibility is required.
| TARGET_LINK_LIBRARIES(cleedplusplus rfac m) | ||
| TARGET_LINK_LIBRARIES(cleedplusplusStatic rfacStatic m) | ||
| TARGET_LINK_LIBRARIES(cleedplusplus m) |
There was a problem hiding this comment.
Duplicate link to m library.
cleedplusplus links against m twice (lines 48 and 50). Remove the redundant link.
🐛 Proposed fix
TARGET_LINK_LIBRARIES(cleedplusplus rfac m)
TARGET_LINK_LIBRARIES(cleedplusplusStatic rfacStatic m)
-TARGET_LINK_LIBRARIES(cleedplusplus m)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TARGET_LINK_LIBRARIES(cleedplusplus rfac m) | |
| TARGET_LINK_LIBRARIES(cleedplusplusStatic rfacStatic m) | |
| TARGET_LINK_LIBRARIES(cleedplusplus m) | |
| TARGET_LINK_LIBRARIES(cleedplusplus rfac m) | |
| TARGET_LINK_LIBRARIES(cleedplusplusStatic rfacStatic m) |
🤖 Prompt for AI Agents
In @src/cleed++/CMakeLists.txt around lines 48 - 50, The CMake file links the
math library `m` twice for the `cleedplusplus` target; remove the redundant
TARGET_LINK_LIBRARIES(cleedplusplus m) entry so `cleedplusplus` is only linked
once to `m` (keep the existing TARGET_LINK_LIBRARIES(cleedplusplus rfac m)
line); ensure you do not remove the separate `cleedplusplusStatic` linkage line
TARGET_LINK_LIBRARIES(cleedplusplusStatic rfacStatic m).
| Atom::Atom(std::vector<double> pos) { | ||
| setPosition(pos); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pass std::vector by const reference to avoid unnecessary copies.
The vector parameter in the constructor and setPosition is passed by value, causing an unnecessary copy.
♻️ Proposed fix
In the header (Atom.hh), update the declarations, then in this file:
-Atom::Atom(std::vector<double> pos) {
+Atom::Atom(const std::vector<double>& pos) {
setPosition(pos);
}-Atom& Atom::setPosition(std::vector<double> pos) {
+Atom& Atom::setPosition(const std::vector<double>& pos) {
setXPosition(pos.at(0));
setYPosition(pos.at(1));
setZPosition(pos.at(2));
return *this;
}Also applies to: 77-81
🤖 Prompt for AI Agents
In @src/cleed++/Core/Atom.cc around lines 34 - 36, The constructor Atom::Atom
and the setPosition method currently take std::vector<double> by value causing
copies; change their signatures to take const std::vector<double>& (update both
the Atom.hh declaration(s) and the Atom.cc definitions) and adjust any callers
if necessary so you pass by const reference; ensure implementation bodies use
the referenced parameter without attempting to modify it (or make a non-const
local copy only when needed).
| std::vector<double> Atom::getPosition() const { | ||
| std::vector<double> pos; | ||
| pos.push_back(this->x); // X | ||
| pos.push_back(this->y); // Y | ||
| pos.push_back(this->z); // Z | ||
| return pos; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using initializer list for getPosition return.
Using an initializer list is more concise and avoids multiple push_back calls.
♻️ Proposed simplification
std::vector<double> Atom::getPosition() const {
- std::vector<double> pos;
- pos.push_back(this->x); // X
- pos.push_back(this->y); // Y
- pos.push_back(this->z); // Z
- return pos;
+ return {this->x, this->y, this->z};
}🤖 Prompt for AI Agents
In @src/cleed++/Core/Atom.cc around lines 54 - 60, The getPosition function in
Atom (Atom::getPosition) currently builds the std::vector<double> using multiple
push_back calls; replace that with direct list-initialization and return, e.g.
construct and return std::vector<double>{this->x, this->y, this->z} (or {x, y,
z}) to make the code more concise and avoid repeated push_back calls.
| inline bool BasicElement::operator!() { | ||
| return (this != nullptr); | ||
| } |
There was a problem hiding this comment.
operator!() has inverted logic and is always true.
Comparing this != nullptr inside a member function is always true (undefined behavior if this were null). The logical negation operator should typically check if the element is invalid.
Proposed fix
inline bool BasicElement::operator!() {
- return (this != nullptr);
+ return (this->Z == 0 || this->Z > ELEMENTS.size());
}🤖 Prompt for AI Agents
In @src/cleed++/Core/BasicElement.cc around lines 389 - 391,
BasicElement::operator!() currently compares this to nullptr (this != nullptr)
which is always true and invalid; replace that check with a real validity test
so operator! returns true when the element is invalid. Modify
BasicElement::operator!() to return the negation of the class's validity
indicator (for example, use !isValid(), or if an operator bool() or an internal
flag like m_valid exists use its negation), removing the bogus this pointer
comparison.
| BasicElement &BasicElement::operator-(int Z) { | ||
| if (getAtomicNumber() - Z < 0) { | ||
| throw invalidElementException("Atomic number Z is not valid"); | ||
| } | ||
| return setAtomicNumber(getAtomicNumber() - Z); | ||
| } |
There was a problem hiding this comment.
Unsigned comparison with < 0 is always false.
getAtomicNumber() returns int (which appears to be from an unsigned Z member), and checking getAtomicNumber() - Z < 0 when both are unsigned types will never be true due to unsigned wraparound.
Proposed fix
BasicElement &BasicElement::operator-(int Z) {
- if (getAtomicNumber() - Z < 0) {
+ if (Z > static_cast<int>(getAtomicNumber())) {
throw invalidElementException("Atomic number Z is not valid");
}
return setAtomicNumber(getAtomicNumber() - Z);
}🤖 Prompt for AI Agents
In @src/cleed++/Core/BasicElement.cc around lines 407 - 412, The subtraction
operator uses an unsigned-aware expression so the check `getAtomicNumber() - Z <
0` will never trigger; update BasicElement::operator-(int Z) to validate by
comparing Z against the current atomic number (e.g., if (Z > getAtomicNumber())
throw invalidElementException(...)) before performing the subtraction and then
call setAtomicNumber(getAtomicNumber() - Z); keep references to
BasicElement::operator-, getAtomicNumber(), setAtomicNumber(), and
invalidElementException when making the change.
| Search(const string &control_filepath, | ||
| const string &theory_filepath, | ||
| search_method driver=SR_SIMPLEX); | ||
| ~Search(); |
There was a problem hiding this comment.
Destructor should be virtual since class has virtual methods.
The class has virtual methods (startSearch, stopSearch, resumeSearch), indicating it may be used as a base class. A non-virtual destructor can cause resource leaks when deleting derived objects through a base pointer.
🐛 Proposed fix
- ~Search();
+ virtual ~Search();Also applies to: 120-122
🤖 Prompt for AI Agents
In @src/cleed++/Search/Search.hh at line 42, Change the non-virtual destructor
declaration "~Search();" to a virtual destructor ("virtual ~Search();") so
derived objects are properly destroyed when deleted via a Search*; also inspect
the other class destructors in this file that have virtual member functions (the
additional destructors called out in the review) and make them virtual as well
to avoid slicing/resource leaks.
| SearchAtom::SearchAtom() { | ||
| this->x_par = nullptr; | ||
| this->y_par = nullptr; | ||
| this->z_par = nullptr; | ||
| this->dr_par = nullptr; | ||
|
|
||
| this->setXPosition(0.); | ||
| this->setYPosition(0.); | ||
| this->setZPosition(0.); | ||
| this->setAtomName("\0"); | ||
| } |
There was a problem hiding this comment.
Default constructor doesn't initialize all inherited members.
Only the pointer members and position are initialized. Members like type, nref, ref, r_min, and dr remain uninitialized, which could cause issues when accessed via getters.
Proposed fix
SearchAtom::SearchAtom() {
this->x_par = nullptr;
this->y_par = nullptr;
this->z_par = nullptr;
this->dr_par = nullptr;
+ this->type = 0;
+ this->nref = 0;
+ this->ref = 0;
+ this->r_min = 0.0;
+ this->dr = 0.0;
this->setXPosition(0.);
this->setYPosition(0.);
this->setZPosition(0.);
this->setAtomName("\0");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SearchAtom::SearchAtom() { | |
| this->x_par = nullptr; | |
| this->y_par = nullptr; | |
| this->z_par = nullptr; | |
| this->dr_par = nullptr; | |
| this->setXPosition(0.); | |
| this->setYPosition(0.); | |
| this->setZPosition(0.); | |
| this->setAtomName("\0"); | |
| } | |
| SearchAtom::SearchAtom() { | |
| this->x_par = nullptr; | |
| this->y_par = nullptr; | |
| this->z_par = nullptr; | |
| this->dr_par = nullptr; | |
| this->type = 0; | |
| this->nref = 0; | |
| this->ref = 0; | |
| this->r_min = 0.0; | |
| this->dr = 0.0; | |
| this->setXPosition(0.); | |
| this->setYPosition(0.); | |
| this->setZPosition(0.); | |
| this->setAtomName("\0"); | |
| } |
🤖 Prompt for AI Agents
In @src/cleed++/Search/SearchAtom.cc around lines 29 - 39, The constructor
leaves inherited members uninitialized; update SearchAtom::SearchAtom() to
initialize type, nref, ref, r_min and dr (either via available setters or
directly) — for example call setType(0), setNRef(0), setRef(nullptr),
setRMin(0.0) and setDR(0.0) (or assign this->type = 0; this->nref = 0; this->ref
= nullptr; this->r_min = 0.0; this->dr = 0.0) alongside the existing
setXPosition/setYPosition/setZPosition/setAtomName calls so all inherited state
is deterministic.
| SearchAtom::SearchAtom(const SearchAtom &other) { | ||
|
|
||
| } |
There was a problem hiding this comment.
Empty copy constructor fails to copy any state.
The copy constructor body is empty, leaving the new object uninitialized. This will cause undefined behavior when the copy is used or destroyed (double-free or use of uninitialized pointers).
Proposed fix
SearchAtom::SearchAtom(const SearchAtom &other) {
-
+ this->x_par = nullptr;
+ this->y_par = nullptr;
+ this->z_par = nullptr;
+ this->dr_par = nullptr;
+
+ this->type = other.type;
+ this->nref = other.nref;
+ this->ref = other.ref;
+ this->r_min = other.r_min;
+ this->x = other.x;
+ this->y = other.y;
+ this->z = other.z;
+ this->dr = other.dr;
+ std::strncpy(this->name, other.name, STRSZ - 1);
+ this->name[STRSZ - 1] = '\0';
+
+ // Deep copy the parameter arrays if they exist
+ if (other.x_par && other.nref > 0) {
+ this->x_par = (double*)std::malloc(other.nref * sizeof(double));
+ std::memcpy(this->x_par, other.x_par, other.nref * sizeof(double));
+ }
+ // Similarly for y_par, z_par, dr_par...
}🤖 Prompt for AI Agents
In @src/cleed++/Search/SearchAtom.cc around lines 41 - 43, The SearchAtom copy
constructor is empty and does not copy any state, causing undefined behavior;
either implement it to perform a proper member-wise (or deep) copy of all
relevant members of SearchAtom (ensuring pointer/ownership semantics are handled
to avoid double-free), or replace the user-defined constructor with a
compiler-generated one by marking SearchAtom::SearchAtom(const SearchAtom&) =
default; if copying is not intended, explicitly delete it
(SearchAtom::SearchAtom(const SearchAtom&) = delete); update any raw-pointer
members to use safe ownership (unique_ptr/shared_ptr) or explicitly clone their
contents inside the copy constructor.
| inline std::vector<double> SearchAtom::getXShifts() { | ||
| return std::vector<double>(this->x_par, this->x_par + this->nref); | ||
| } | ||
|
|
||
| inline std::vector<double> SearchAtom::getYShifts() { | ||
| return std::vector<double>(this->y_par, this->y_par + this->nref); | ||
| } | ||
|
|
||
| std::vector<double> SearchAtom::getZShifts() { | ||
| return std::vector<double>(this->z_par, this->z_par + this->nref); | ||
| } | ||
|
|
||
| std::vector<double> SearchAtom::getDeltaRShifts() { | ||
| return std::vector<double>(this->dr_par, this->dr_par + this->nref); | ||
| } |
There was a problem hiding this comment.
Potential null pointer dereference and uninitialized nref access.
getXShifts, getYShifts, getZShifts, and getDeltaRShifts construct vectors from potentially null pointers (x_par, etc.) using this->nref which may be uninitialized. This causes undefined behavior.
Proposed fix - add null checks
inline std::vector<double> SearchAtom::getXShifts() {
+ if (!this->x_par || this->nref == 0) {
+ return std::vector<double>();
+ }
return std::vector<double>(this->x_par, this->x_par + this->nref);
}Apply similar guards to getYShifts, getZShifts, and getDeltaRShifts.
🤖 Prompt for AI Agents
In @src/cleed++/Search/SearchAtom.cc around lines 89 - 103, The four accessors
getXShifts, getYShifts, getZShifts and getDeltaRShifts currently build vectors
from raw pointers (x_par, y_par, z_par, dr_par) using this->nref which may be
uninitialized or the pointers may be null; add defensive checks at the start of
each function to return an empty std::vector<double>() if the respective pointer
is null or if nref is <= 0 (or otherwise invalid), and only construct the vector
from pointer..pointer+nref when both the pointer is non-null and nref is a valid
positive size (cast nref to size_t if needed); update all four functions
(referencing x_par, y_par, z_par, dr_par and nref) accordingly.
| int rfac_iv_to_arrays(const rfac_iv *iv, double *x, double *fx, size_t *n) | ||
| { | ||
| size_t i; | ||
|
|
||
| /* allocate memory */ | ||
| if (x != NULL) CLEED_REALLOC(x, iv->n_eng * sizeof(double)); | ||
| else x = (double *) malloc(iv->n_eng * sizeof(double)); | ||
|
|
||
| if (x == NULL) return (RFAC_ALLOCATION_ERROR); | ||
|
|
||
| if (fx != NULL) CLEED_REALLOC(fx, iv->n_eng * sizeof(double)); | ||
| else fx = (double *) malloc(iv->n_eng * sizeof(double)); | ||
|
|
||
| if (fx == NULL) | ||
| { | ||
| free(x); | ||
| return (RFAC_ALLOCATION_ERROR); | ||
| } | ||
|
|
||
| /* convert energies */ | ||
| for (i=0; i<iv->n_eng; i++) | ||
| { | ||
| x[i] = iv->data[i].energy; | ||
| fx[i] = iv->data[i].intens; | ||
| } | ||
|
|
||
| *n = iv->n_eng; | ||
|
|
||
| return (0); |
There was a problem hiding this comment.
rfac_iv_to_arrays doesn't capture CLEED_REALLOC result - memory corruption.
Lines 253 and 258 call CLEED_REALLOC but don't assign the result back to x and fx. If realloc moves the memory, the original pointers become dangling. Also, these are local copies of the pointers - the caller's pointers won't be updated.
Proposed fix - use double pointers
-int rfac_iv_to_arrays(const rfac_iv *iv, double *x, double *fx, size_t *n)
+int rfac_iv_to_arrays(const rfac_iv *iv, double **x, double **fx, size_t *n)
{
size_t i;
/* allocate memory */
- if (x != NULL) CLEED_REALLOC(x, iv->n_eng * sizeof(double));
- else x = (double *) malloc(iv->n_eng * sizeof(double));
+ if (*x != NULL) {
+ double *tmp = (double *) realloc(*x, iv->n_eng * sizeof(double));
+ if (tmp == NULL) return (RFAC_ALLOCATION_ERROR);
+ *x = tmp;
+ } else {
+ *x = (double *) malloc(iv->n_eng * sizeof(double));
+ }
- if (x == NULL) return (RFAC_ALLOCATION_ERROR);
+ if (*x == NULL) return (RFAC_ALLOCATION_ERROR);
- if (fx != NULL) CLEED_REALLOC(fx, iv->n_eng * sizeof(double));
- else fx = (double *) malloc(iv->n_eng * sizeof(double));
+ if (*fx != NULL) {
+ double *tmp = (double *) realloc(*fx, iv->n_eng * sizeof(double));
+ if (tmp == NULL) {
+ free(*x);
+ *x = NULL;
+ return (RFAC_ALLOCATION_ERROR);
+ }
+ *fx = tmp;
+ } else {
+ *fx = (double *) malloc(iv->n_eng * sizeof(double));
+ }
- if (fx == NULL)
+ if (*fx == NULL)
{
- free(x);
+ free(*x);
+ *x = NULL;
return (RFAC_ALLOCATION_ERROR);
}
/* convert energies */
for (i=0; i<iv->n_eng; i++)
{
- x[i] = iv->data[i].energy;
- fx[i] = iv->data[i].intens;
+ (*x)[i] = iv->data[i].energy;
+ (*fx)[i] = iv->data[i].intens;
}
*n = iv->n_eng;
return (0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int rfac_iv_to_arrays(const rfac_iv *iv, double *x, double *fx, size_t *n) | |
| { | |
| size_t i; | |
| /* allocate memory */ | |
| if (x != NULL) CLEED_REALLOC(x, iv->n_eng * sizeof(double)); | |
| else x = (double *) malloc(iv->n_eng * sizeof(double)); | |
| if (x == NULL) return (RFAC_ALLOCATION_ERROR); | |
| if (fx != NULL) CLEED_REALLOC(fx, iv->n_eng * sizeof(double)); | |
| else fx = (double *) malloc(iv->n_eng * sizeof(double)); | |
| if (fx == NULL) | |
| { | |
| free(x); | |
| return (RFAC_ALLOCATION_ERROR); | |
| } | |
| /* convert energies */ | |
| for (i=0; i<iv->n_eng; i++) | |
| { | |
| x[i] = iv->data[i].energy; | |
| fx[i] = iv->data[i].intens; | |
| } | |
| *n = iv->n_eng; | |
| return (0); | |
| int rfac_iv_to_arrays(const rfac_iv *iv, double **x, double **fx, size_t *n) | |
| { | |
| size_t i; | |
| /* allocate memory */ | |
| if (*x != NULL) { | |
| double *tmp = (double *) realloc(*x, iv->n_eng * sizeof(double)); | |
| if (tmp == NULL) return (RFAC_ALLOCATION_ERROR); | |
| *x = tmp; | |
| } else { | |
| *x = (double *) malloc(iv->n_eng * sizeof(double)); | |
| } | |
| if (*x == NULL) return (RFAC_ALLOCATION_ERROR); | |
| if (*fx != NULL) { | |
| double *tmp = (double *) realloc(*fx, iv->n_eng * sizeof(double)); | |
| if (tmp == NULL) { | |
| free(*x); | |
| *x = NULL; | |
| return (RFAC_ALLOCATION_ERROR); | |
| } | |
| *fx = tmp; | |
| } else { | |
| *fx = (double *) malloc(iv->n_eng * sizeof(double)); | |
| } | |
| if (*fx == NULL) | |
| { | |
| free(*x); | |
| *x = NULL; | |
| return (RFAC_ALLOCATION_ERROR); | |
| } | |
| /* convert energies */ | |
| for (i=0; i<iv->n_eng; i++) | |
| { | |
| (*x)[i] = iv->data[i].energy; | |
| (*fx)[i] = iv->data[i].intens; | |
| } | |
| *n = iv->n_eng; | |
| return (0); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 248-248: parameter name 'iv' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 248-248: parameter name 'x' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 248-248: parameter name 'fx' is too short, expected at least 3 characters
(readability-identifier-length)
🪛 Cppcheck (2.19.0)
[style] 248-248: The function 'rfac_iv_to_arrays' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In @src/rfac/rfac.c around lines 248 - 276, rfac_iv_to_arrays currently ignores
CLEED_REALLOC return values and takes single-pointer args, risking dangling
pointers and failing to update the caller; change the API to accept double
pointers (double **x, double **fx) and inside rfac_iv_to_arrays assign the
result of CLEED_REALLOC/malloc to temporary pointers then store them into *x and
*fx only after success, check for NULL and free any allocated memory on error,
write values into (*x)[i] and (*fx)[i], set *n = iv->n_eng, and return errors as
before so callers receive the updated buffer addresses safely.
| #ifndef __BASICELEMENT_HH__ | ||
| #define __BASICELEMENT_HH__ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Include guard uses reserved identifier.
Identifiers starting with double underscores are reserved for the implementation. Use a different naming convention.
♻️ Proposed fix
-#ifndef __BASICELEMENT_HH__
-#define __BASICELEMENT_HH__
+#ifndef CLEED_CORE_BASICELEMENT_HH
+#define CLEED_CORE_BASICELEMENT_HHAnd at the end of the file:
-#endif /* __BASICELEMENT_HH__ */
+#endif /* CLEED_CORE_BASICELEMENT_HH */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifndef __BASICELEMENT_HH__ | |
| #define __BASICELEMENT_HH__ | |
| #ifndef CLEED_CORE_BASICELEMENT_HH | |
| #define CLEED_CORE_BASICELEMENT_HH |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 19-19: declaration uses identifier 'BASICELEMENT_HH', which is a reserved identifier
(bugprone-reserved-identifier)
🤖 Prompt for AI Agents
In @src/cleed++/Core/BasicElement.hh around lines 18 - 19, The include guard
currently uses the reserved identifier __BASICELEMENT_HH__; change it to a
non-reserved macro (for example BASICELEMENT_HH or CLEED_BASICELEMENT_HH)
wherever it appears (the #ifndef and #define at the top and the matching #endif
comment at the end) and ensure the new macro name is used consistently across
the file (e.g., in the #ifndef, #define, and trailing #endif comment).
| inline int Element::getGroup() const { | ||
| return group; | ||
| } | ||
|
|
||
| inline int Element::getPeriod() const { | ||
| return period; | ||
| } | ||
|
|
||
| inline char Element::getBlock() const { | ||
| return block; | ||
| } | ||
|
|
||
| inline int Element::getSeries() const { | ||
| return series; | ||
| } | ||
|
|
||
| inline double Element::getMolarMass() const { | ||
| return molarMass; | ||
| } | ||
|
|
||
| inline double Element::getElectronegivity() const { | ||
| return electronegativity; | ||
| } | ||
|
|
||
| inline double Element::getElectronAffinity() const { | ||
| return electronAffinity; | ||
| } | ||
|
|
||
| inline double Element::getCovalentRadius() const { | ||
| return covalentRadius; | ||
| } | ||
|
|
||
| inline double Element::getAtomicRadius() const { | ||
| return atomicRadius; | ||
| } | ||
|
|
||
| inline double Element::getVanDerWaalsRadius() const { | ||
| return vdwRadius; | ||
| } | ||
|
|
||
| inline double Element::getBoilingPoint() const { | ||
| return boilingPoint; | ||
| } | ||
|
|
||
| inline double Element::getMeltingPoint() const { | ||
| return meltingPoint; | ||
| } | ||
|
|
||
| inline double Element::getDensity() const { | ||
| return density; | ||
| } | ||
|
|
||
| inline const std::string &Element::getElectronConfiguration() const { | ||
| return electronConfig; | ||
| } | ||
|
|
||
| inline const std::string &Element::getOxidationStates() const { | ||
| return oxidationStates; | ||
| } | ||
|
|
||
| inline const std::vector<double> &Element::getIonisationEnergies() const { | ||
| return ionisationEnergies; | ||
| } | ||
|
|
||
| inline const std::string &Element::getDescription() const { | ||
| return description; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
inline on function definitions in .cc files has no effect for external callers.
These functions are marked inline but defined in the .cc file, so the inline hint only applies within this translation unit. If inlining across TUs is desired, move definitions to the header. Otherwise, remove the inline keyword for clarity.
🤖 Prompt for AI Agents
In @src/cleed++/Core/Element.cc around lines 92 - 158, The getters in Element.cc
(e.g., Element::getGroup, Element::getPeriod, Element::getBlock,
Element::getSeries, Element::getMolarMass, Element::getElectronegivity,
Element::getElectronAffinity, Element::getCovalentRadius,
Element::getAtomicRadius, Element::getVanDerWaalsRadius,
Element::getBoilingPoint, Element::getMeltingPoint, Element::getDensity,
Element::getElectronConfiguration, Element::getOxidationStates,
Element::getIonisationEnergies, Element::getDescription) are marked inline but
defined in the .cc so the inline has no TU-wide effect; either move these
function definitions into the Element class header (where inline will be honored
across TUs) or simply remove the inline keyword from each definition in
Element.cc to keep the implementation clear and consistent.
| inline double Element::getElectronegivity() const { | ||
| return electronegativity; | ||
| } |
There was a problem hiding this comment.
Getter name typo matches header—both need fixing.
getElectronegivity should be getElectronegativity to match the setter and correct spelling.
🤖 Prompt for AI Agents
In @src/cleed++/Core/Element.cc around lines 112 - 114, The getter is misspelled
as getElectronegivity; rename it to getElectronegativity in the Element class
declaration and definition (replace the method named getElectronegivity with
getElectronegativity) and update any call sites to use getElectronegativity so
it matches the setter and correct spelling.
| throw invalidElementException(("invalid chemical group: " + group).c_str()); | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Unreachable code after throw.
The return *this; on line 177 is unreachable since throw transfers control unconditionally. Same issue exists in setPeriod (line 200) and setSeries (line 238).
Proposed fix for all three methods
throw invalidElementException(("invalid chemical group: " + group).c_str());
- return *this;
}Apply similar fix to lines 199-200 and 237-238.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw invalidElementException(("invalid chemical group: " + group).c_str()); | |
| return *this; | |
| } | |
| throw invalidElementException(("invalid chemical group: " + group).c_str()); | |
| } |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 177-177: src/cleed++/Core/Element.cc#L177
Consecutive return, break, continue, goto or throw statements are unnecessary.
| Element &Element::setBoilingPoint(double tboil) { | ||
| this->boilingPoint = (tboil > 0.) ? tboil : std::abs(tboil); | ||
| return *this; | ||
| } | ||
|
|
||
| Element &Element::setMeltingPoint(double tmelt) { | ||
| this->meltingPoint = (tmelt > 0.) ? tmelt : std::abs(tmelt); | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Silent coercion of negative temperatures is surprising behavior.
setBoilingPoint and setMeltingPoint silently convert negative values to positive using std::abs(). This could mask bugs where callers accidentally pass negative values. Consider either:
- Throwing an exception for invalid input (consistent with other setters)
- Documenting this coercion behavior clearly
🤖 Prompt for AI Agents
In @src/cleed++/Core/Element.cc around lines 287 - 295, The setters
Element::setBoilingPoint and Element::setMeltingPoint currently silently coerce
negative inputs to positive using std::abs, which is surprising; change them to
validate the input and handle negatives consistently with other setters — e.g.,
if tmelt or tboil is negative, throw an std::invalid_argument (or the project's
chosen exception type) with a clear message rather than converting, or if you
prefer to keep coercion, add a documentation comment and an explicit conversion
log; update Element::setBoilingPoint and Element::setMeltingPoint to perform the
chosen behavior so callers cannot be silently masked.
| vector<double> &getMirrorPlaneGeometry() const; | ||
| int getMirrorSymmetry() const; | ||
| const real *getAngleAlphaDegreesPtr() const; | ||
| vector<double> &getAngleAlphaDegrees() const; |
There was a problem hiding this comment.
Returning non-const reference from const method is problematic.
getMirrorPlaneGeometry() and getAngleAlphaDegrees() are declared const but return vector<double>&. This allows modification of state through a const object. Looking at the implementation in Crystal.cc, these return references to static variables—which is thread-unsafe and semantically incorrect.
Either return by value or return const vector<double>& if caching is truly needed.
Proposed fix
- vector<double> &getMirrorPlaneGeometry() const;
+ vector<double> getMirrorPlaneGeometry() const;
int getMirrorSymmetry() const;
const real *getAngleAlphaDegreesPtr() const;
- vector<double> &getAngleAlphaDegrees() const;
+ vector<double> getAngleAlphaDegrees() const;🧰 Tools
🪛 Clang (14.0.6)
[warning] 61-61: function 'getMirrorPlaneGeometry' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 61-61: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 62-62: function 'getMirrorSymmetry' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 62-62: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 63-63: function 'getAngleAlphaDegreesPtr' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 63-63: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 64-64: function 'getAngleAlphaDegrees' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 64-64: use a trailing return type for this function
(modernize-use-trailing-return-type)
🤖 Prompt for AI Agents
In @src/cleed++/LEED/Crystal.hh around lines 61 - 64, The methods
getMirrorPlaneGeometry() and getAngleAlphaDegrees() currently declare const but
return mutable vector<double>& and their implementations return references to
static variables (thread-unsafe and breaks const-correctness); change their
signatures to either return a const vector<double>& or return vector<double> by
value (prefer by-value if simplicity and thread-safety are desired), update the
corresponding implementations in Crystal.cc to return a local or member vector
(not a static) matching the new signature, and adjust any callers to accept a
const reference or a value as appropriate; ensure const correctness is preserved
for getMirrorSymmetry() and getAngleAlphaDegreesPtr() usages.
|
|
||
| size_t getNumberOfLayers() const; | ||
| const leed_layer_t *getLayersPtr() const; | ||
| vector<leed_layer_t> &getLayers() const; |
There was a problem hiding this comment.
Same issue: getLayers() returns non-const reference from const method.
This has the same thread-safety and semantic issues as the other getters returning references to static variables.
Proposed fix
- vector<leed_layer_t> &getLayers() const;
+ vector<leed_layer_t> getLayers() const;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vector<leed_layer_t> &getLayers() const; | |
| vector<leed_layer_t> getLayers() const; |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 73-73: function 'getLayers' should be marked [[nodiscard]]
(modernize-use-nodiscard)
[warning] 73-73: use a trailing return type for this function
(modernize-use-trailing-return-type)
🤖 Prompt for AI Agents
In @src/cleed++/LEED/Crystal.hh at line 73, getLayers() is declared as a const
method but returns a non-const reference, exposing mutable access from a const
object and repeating the thread-safety/semantic issue; change the signature to
return a const reference (const std::vector<leed_layer_t>& getLayers() const) or
alternatively return by value if you must protect internal/static state, and
update callers accordingly to avoid modifying the internal layers through a
const object.
| PhaseShift& PhaseShift::setData(const real *energies_ptr, | ||
| const real *phaseshifts_ptr, std::size_t n) { | ||
| try { | ||
| if (this->energy != nullptr) { | ||
| std::free(static_cast<void*>(this->energy)); | ||
| } | ||
| if (this->pshift != nullptr) { | ||
| std::free(static_cast<void*>(this->pshift)); | ||
| } | ||
| this->energy = static_cast<real*>(std::calloc(n, sizeof(real))); | ||
| this->pshift = static_cast<real*>(std::calloc(n, sizeof(real))); | ||
| std::copy(energies_ptr, energies_ptr + n, this->energy); | ||
| std::copy(phaseshifts_ptr, phaseshifts_ptr + n, this->pshift); | ||
| this->neng = n; | ||
| } catch (...) { | ||
| //!TODO | ||
| } | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
setData dereferences potentially null pointers after failed allocation.
If std::calloc returns nullptr (out of memory), std::copy will dereference null pointers causing a crash. The try-catch won't help since std::copy to a null pointer is undefined behavior, not an exception.
🐛 Proposed fix
PhaseShift& PhaseShift::setData(const real *energies_ptr,
const real *phaseshifts_ptr, std::size_t n) {
- try {
- if (this->energy != nullptr) {
- std::free(static_cast<void*>(this->energy));
- }
- if (this->pshift != nullptr) {
- std::free(static_cast<void*>(this->pshift));
- }
- this->energy = static_cast<real*>(std::calloc(n, sizeof(real)));
- this->pshift = static_cast<real*>(std::calloc(n, sizeof(real)));
+ if (n == 0 || energies_ptr == nullptr || phaseshifts_ptr == nullptr) {
+ return *this;
+ }
+
+ real* new_energy = static_cast<real*>(std::calloc(n, sizeof(real)));
+ real* new_pshift = static_cast<real*>(std::calloc(n, sizeof(real)));
+
+ if (new_energy == nullptr || new_pshift == nullptr) {
+ std::free(new_energy);
+ std::free(new_pshift);
+ return *this; // or throw std::bad_alloc()
+ }
+
+ // Free old data only after successful allocation
+ std::free(this->energy);
+ std::free(this->pshift);
+
+ this->energy = new_energy;
+ this->pshift = new_pshift;
std::copy(energies_ptr, energies_ptr + n, this->energy);
std::copy(phaseshifts_ptr, phaseshifts_ptr + n, this->pshift);
this->neng = n;
- } catch (...) {
- //!TODO
- }
return *this;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PhaseShift& PhaseShift::setData(const real *energies_ptr, | |
| const real *phaseshifts_ptr, std::size_t n) { | |
| try { | |
| if (this->energy != nullptr) { | |
| std::free(static_cast<void*>(this->energy)); | |
| } | |
| if (this->pshift != nullptr) { | |
| std::free(static_cast<void*>(this->pshift)); | |
| } | |
| this->energy = static_cast<real*>(std::calloc(n, sizeof(real))); | |
| this->pshift = static_cast<real*>(std::calloc(n, sizeof(real))); | |
| std::copy(energies_ptr, energies_ptr + n, this->energy); | |
| std::copy(phaseshifts_ptr, phaseshifts_ptr + n, this->pshift); | |
| this->neng = n; | |
| } catch (...) { | |
| //!TODO | |
| } | |
| return *this; | |
| } | |
| PhaseShift& PhaseShift::setData(const real *energies_ptr, | |
| const real *phaseshifts_ptr, std::size_t n) { | |
| if (n == 0 || energies_ptr == nullptr || phaseshifts_ptr == nullptr) { | |
| return *this; | |
| } | |
| real* new_energy = static_cast<real*>(std::calloc(n, sizeof(real))); | |
| real* new_pshift = static_cast<real*>(std::calloc(n, sizeof(real))); | |
| if (new_energy == nullptr || new_pshift == nullptr) { | |
| std::free(new_energy); | |
| std::free(new_pshift); | |
| return *this; // or throw std::bad_alloc() | |
| } | |
| // Free old data only after successful allocation | |
| std::free(this->energy); | |
| std::free(this->pshift); | |
| this->energy = new_energy; | |
| this->pshift = new_pshift; | |
| std::copy(energies_ptr, energies_ptr + n, this->energy); | |
| std::copy(phaseshifts_ptr, phaseshifts_ptr + n, this->pshift); | |
| this->neng = n; | |
| return *this; | |
| } |
🤖 Prompt for AI Agents
In @src/cleed++/LEED/PhaseShift.cc around lines 191 - 209, PhaseShift::setData
currently calls std::calloc and then uses std::copy into
this->energy/this->pshift without checking for allocation failure; change it to
allocate into temporary pointers (e.g., real* new_energy and real* new_pshift),
verify both are non-null (handle failure by freeing any partial temp allocation
and returning or signaling error), only after both allocations succeed copy the
input data into the temps, free the old this->energy/this->pshift, assign the
temps to these members, and update this->neng — this prevents dereferencing null
pointers and ensures state is only updated on success.
| #ifndef END_OF_GROUP_ID | ||
| #define END_OF_GROUP_ID -1 | ||
| #endif |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider parenthesizing macro value.
While -1 alone is safe, wrapping macro values in parentheses is a defensive best practice.
Proposed fix
#ifndef END_OF_GROUP_ID
-#define END_OF_GROUP_ID -1
+#define END_OF_GROUP_ID (-1)
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifndef END_OF_GROUP_ID | |
| #define END_OF_GROUP_ID -1 | |
| #endif | |
| #ifndef END_OF_GROUP_ID | |
| #define END_OF_GROUP_ID (-1) | |
| #endif |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 141-141: macro replacement list should be enclosed in parentheses
(bugprone-macro-parentheses)
🤖 Prompt for AI Agents
In @src/include/rfac_def.h around lines 140 - 142, The macro END_OF_GROUP_ID is
defined as -1 without parentheses; change its definition to wrap the value in
parentheses to avoid unexpected precedence issues by redefining the macro
(END_OF_GROUP_ID) to ( -1 ) — update the #define for END_OF_GROUP_ID accordingly
so all uses expand with parentheses around the literal.
| typedef struct rfac_ivcur | ||
| { | ||
| int group_id; | ||
| real eng_0; | ||
| rfac_spot spot_id; | ||
| rfac_iv *theory; | ||
| rfac_iv *experimental; | ||
| real overlap; | ||
| rfac rfac; | ||
| real weight; | ||
| } rfac_ivcur; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Field name shadows type name: rfac rfac;
On line 172, the field is named rfac which is also the typedef name. This is legal C but can cause confusion and potential issues in some contexts. Consider renaming the field to something like rfac_result or r_factors.
Proposed fix
typedef struct rfac_ivcur
{
int group_id;
real eng_0;
rfac_spot spot_id;
rfac_iv *theory;
rfac_iv *experimental;
real overlap;
- rfac rfac;
+ rfac r_factors;
real weight;
} rfac_ivcur;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| typedef struct rfac_ivcur | |
| { | |
| int group_id; | |
| real eng_0; | |
| rfac_spot spot_id; | |
| rfac_iv *theory; | |
| rfac_iv *experimental; | |
| real overlap; | |
| rfac rfac; | |
| real weight; | |
| } rfac_ivcur; | |
| typedef struct rfac_ivcur | |
| { | |
| int group_id; | |
| real eng_0; | |
| rfac_spot spot_id; | |
| rfac_iv *theory; | |
| rfac_iv *experimental; | |
| real overlap; | |
| rfac r_factors; | |
| real weight; | |
| } rfac_ivcur; |
🤖 Prompt for AI Agents
In @src/include/rfac_def.h around lines 164 - 174, The struct rfac_ivcur
declares a field named rfac that shadows the typedef rfac; rename that field
(e.g., to rfac_result or r_factors) inside rfac_ivcur and update all
references/usages of that field throughout the codebase (constructors,
assignments, accessors, serializers, etc.) to the new name; ensure any related
comments or API documentation and any initializers in functions that populate
rfac_ivcur are updated accordingly to avoid ambiguity with the rfac type.
On Windows, the search and rfac shared libraries don't export symbols properly (missing __declspec(dllexport)) and thus don't generate import libraries (.lib files). Link cleedCore against the static versions on Windows to fix LNK1104 error.
Same issue as cleedCore - Windows DLLs need __declspec(dllexport) to generate import libraries. Use static linking on Windows instead.
There was a problem hiding this comment.
Actionable comments posted: 45
🤖 Fix all issues with AI agents
In @src/cleed_core/CMakeLists.txt:
- Around line 28-39: The Windows install block for the cleedCore target omits an
ARCHIVE destination so the import library (.lib) won't be installed; update the
WIN32 install(TARGETS cleedCore ...) invocation to include "ARCHIVE DESTINATION
bin" alongside the existing RUNTIME and LIBRARY DESTINATION bin entries so the
import library is installed for consumers to link against the DLL (i.e., add
ARCHIVE DESTINATION bin in the WIN32 branch of the install call for cleedCore).
In @src/rfac/rfac.c:
- Around line 145-154: The function rfac_iv_free currently sets the local
parameter _iv to NULL after freeing, which does not affect the caller and is
misleading; either remove the pointless assignment "_iv = NULL" from
rfac_iv_free, or change the API to accept a double pointer (rfac_iv **_iv) so
you free(*_iv), set * _iv = NULL, and update all callers to pass the address of
their pointer. Ensure the chosen approach updates function signature and call
sites consistently if you pick the double-pointer option.
- Around line 164-190: The rfac_iv_copy function leaks memory and invokes
undefined behavior when dest is NULL because it allocates into the local dest
and then reads dest->data uninitialized; fix by changing the API or
initialization: either (preferred) change rfac_iv_copy to return rfac_iv* (or
provide a separate rfac_iv_clone function) so it can allocate a new rfac_iv for
the caller when dest is NULL, or require dest non-NULL and assert/return error;
when allocating a new rfac_iv ensure you zero-initialize it (use calloc or
memset) so dest->data is NULL before any free, copy the struct fields from src
into dest except the data pointer, then allocate dest->data based on src->n_eng
and handle allocation failure by freeing any allocated memory and returning NULL
(or an error), and finally memcpy the data from src->data to dest->data; refer
to function name rfac_iv_copy and members dest->data and dest->n_eng when making
these changes.
- Around line 215-227: The function rfac_ivcur_free_all currently never advances
the pointer and wrongly calls rfac_ivcur_free on each array element (which
itself calls free on the element). Fix by counting elements with an index: loop
while ivcur[i].group_id != END_OF_GROUP_ID incrementing i to get n; then iterate
i from n-1 down to 0 and free only the per-element heap members (e.g. free
ivcur[i].experimental, ivcur[i].theory, and any other allocated members) rather
than freeing &ivcur[i]; finally free the base ivcur pointer. If rfac_ivcur_free
must be reused, change or add a helper (e.g. rfac_ivcur_free_members) that frees
the nested members but does not free the struct itself, and call that from
rfac_ivcur_free_all.
In @src/rfac/rflorentz.c:
- Around line 20-28: The translation unit is missing math.h for sqrt and uses
non-portable malloc.h; update the includes by removing <malloc.h> and adding
<math.h>, keeping the rest unchanged so rfac_iv_lorentz_smooth() can call sqrt()
correctly and allocation uses the standard <stdlib.h>/malloc declarations.
- Around line 114-160: The loop correctly frees intbuf and prefac at the end but
there are duplicated free calls and potential leaks if CLEED_ALLOC_CHECK (or any
early-return) returns before both allocations are done; refactor the function to
use a single-exit cleanup pattern: introduce a cleanup label (e.g., cleanup:)
and replace all intermediate returns (including any early exits after
CLEED_ALLOC_CHECK) with setting an appropriate return code and jumping to
cleanup, ensure intbuf and prefac are only freed once in the cleanup block
(checking for NULL), and remove duplicated free calls inside the normal
equidistant branch so all deallocation happens in that single cleanup section.
- Around line 51-100: The function rfac_iv_lorentz_smooth must validate iv
contents and handle rfac_iv_sort failures: first check iv != NULL (already
present) then ensure iv->n_eng >= 2 before accessing iv->data[1]/iv->data[0] and
return an appropriate error (e.g., RFAC_ALLOCATION_ERROR or a new
RFAC_INVALID_INPUT) if too small; call rfac_iv_sort(iv) and check its return
value, propagating an error if it fails (and avoid continuing), and when
returning early ensure any allocated buffers (intbuf, prefac) are freed; update
all paths that access iv->data to rely on the iv->n_eng check so you never
dereference data[1] when n_eng < 2.
- Around line 90-113: The computed n_range can be zero and can exceed available
buffer sizes; validate and clamp it before writing prefac: after computing
e_step and faux/n_range, if n_range == 0 return RFAC_ESTEP_TOO_SMALL (or handle
to avoid divide-by-zero), and set n_range = MIN(n_range, iv->n_eng + 1) to
prevent OOB; allocate prefac/intbuf based on this capped n_range (or ensure
prefac was allocated for at least iv->n_eng+1 elements) and check norm_sum != 0
before any division. Update the loop that fills prefac[] (in the block using vi,
e_step, SQUARE(...) and the for (i = 0; i < n_range; i++) loop) to use the
capped n_range and ensure all allocations are performed before early returns so
intbuf and prefac are freed on error.
In @src/rfac/rfr1.c:
- Line 49: The function rfac_r1(const real *eng, const real *e_int, const real
*t_int) takes three adjacent pointer parameters of the same type which are easy
to swap; refactor by introducing a small struct (e.g., RfacInputs { const real
*eng; const real *e_int; const real *t_int; }) and change rfac_r1 to accept that
struct (or a pointer to it) and update all call sites to pass the grouped
struct, ensuring arrays are clearly grouped; alternatively, if API change is
undesirable, add runtime validation inside rfac_r1 (e.g., checks that
lengths/terminators of eng, e_int, and t_int match) and assert/fail early with a
clear message referencing the three arrays to prevent silent misordering.
- Line 51: The declaration "int i_eng, n_eng;" combines two variables; split it
into separate declarations for readability and easier future initialization or
comments by replacing it with two lines declaring "int i_eng;" and "int n_eng;"
(optionally initialize or add a comment for each), ensuring you update any
nearby code comments or uses referring to i_eng and n_eng in rfr1.c accordingly.
- Around line 47-48: The docstring for the function rfr1 currently claims it
returns F_FAIL on error but the implementation never returns any failure; either
update the comment to remove the failure claim or implement proper error
handling consistent with the other modules (use RFAC_FAILURE as the failure
sentinel). Specifically, in function rfr1 check for the potential
division-by-zero (the divisor used when computing the R1-factor) and return
RFAC_FAILURE when that check fails (and optionally set/propagate an error
indicator), and update the top-of-function comment to match the chosen behavior.
In @src/rfac/rfr2.c:
- Line 48: The function rfac_r2(eng, e_int, t_int) accepts three adjacent real*
parameters that are easy to mix up; replace them with a single struct (e.g.,
rfac_inputs { const real *eng; const real *e_int; const real *t_int; size_t len;
}) and change rfac_r2 to accept that struct (or a pointer to it) and update all
call sites to construct/pass the struct, or if API compatibility prevents that,
add runtime sanity checks inside rfac_r2: validate non-NULL pointers and that
the three arrays are synchronized (compare lengths or terminators) and
assert/fail early on mismatch; update any callers and tests accordingly to use
the new struct or to ensure arrays are created with a shared length/terminator.
- Line 92: The else clause that sets aux using
cleed_real_sqrt(rfac_sum/norm_sum) is a single-statement without braces; update
the if/else block so the else uses braces (replace "else aux =
cleed_real_sqrt(rfac_sum/norm_sum);" with an else block that contains the
assignment) to improve consistency and prevent future modification errors.
- Line 95: The closing comment incorrectly names the function as cr_r2; update
that comment to match the actual function name rfac_r2 by changing the comment
text at the end of the function (the line containing "end of function cr_r2") to
"end of function rfac_r2" so the symbol rfac_r2 and its closing comment are
consistent.
- Around line 60-70: The code computes norm_te =
cleed_real_sqrt(the_sq_sum/exp_sq_sum) without guarding against exp_sq_sum == 0;
add validation after the loop that checks exp_sq_sum and handles the zero case
(e.g., set norm_te = 0.0 or skip the sqrt and log/return an error) to avoid
division by zero. Specifically, after computing exp_sq_sum, the_sq_sum and
n_eng, verify exp_sq_sum != 0.0 before calling cleed_real_sqrt; if it is zero,
assign a safe default to norm_te and/or handle the error path consistently so
callers of norm_te (and related logic using e_int, t_int, SQUARE) won’t
encounter NaN/inf. Ensure the check references the same variables: exp_sq_sum,
the_sq_sum, norm_te and uses cleed_real_sqrt only when safe.
In @src/rfac/rfrb.c:
- Line 47: The function rfac_rb currently takes three adjacent pointers (eng,
e_int, t_int) that can be confused; refactor by defining a small struct (e.g.,
rfac_inputs { const real *eng; const real *e_int; const real *t_int; }) and
change the signature to accept that struct (rfac_rb(const rfac_inputs *in)) and
update all call sites to pass the struct, or if API changes are undesirable add
runtime assertions inside rfac_rb that validate synchronization (check expected
lengths or sentinel/termination values for eng, e_int, t_int and assert they
match) to prevent swapped arguments. Ensure you update any callers and test
vectors to use the new struct or that the assertions trigger on mismatched
inputs.
- Line 44: The documentation comment incorrectly says "R2-factor" even though
the implementation in rfrb.c implements the Rb1-factor; update the function
comment for the relevant function (rfrb / any function in rfrb.c implementing
the Rb1-factor) to reference "Rb1-factor" and adjust the @return description to
state it returns the Rb1-factor if successful (or #RFAC_FAILURE if failed),
ensuring the term matches the implemented algorithm name.
- Around line 58-66: The code divides rfac_sum by cleed_real_sqrt(the_sq_sum *
exp_sq_sum) without checking for zero, which can cause a divide-by-zero when
either e_int or t_int sums are zero; before the division (right after computing
the_sq_sum and exp_sq_sum in the loop that uses IS_EQUAL_REAL, e_int, t_int,
SQUARE), check whether (the_sq_sum * exp_sq_sum) is zero or nearly zero (use a
small epsilon or exact zero check consistent with cleed_real_sqrt), and if so
handle the edge-case (e.g., set rfac_sum to 0.0 or return an error/flag) instead
of performing the division; place this check immediately before the line that
currently performs rfac_sum /= cleed_real_sqrt(the_sq_sum * exp_sq_sum).
In @src/rfac/rfrmin.c:
- Around line 109-116: The static function rfac_ivcur_get_overlap is currently
unused; either remove it to eliminate dead code or keep it and mark it as
intentionally unused by adding a short comment explaining its planned future use
(e.g., “kept for future SHIFT_DE overlap checks”) or annotate accordingly;
locate the function rfac_ivcur_get_overlap in rfrmin.c and either delete its
definition and any associated macros/headers, or add a clear comment above the
function indicating why it remains (and consider adding an UNUSED/KEEP marker if
your project uses one).
- Around line 45-49: The function rfac_iv_get_energy_range currently only checks
for NULL pointers and can access iv->data[iv->n_eng-1] when iv->n_eng == 0,
causing out-of-bounds access; update rfac_iv_get_energy_range to also check
iv->n_eng == 0 (or iv->n_eng <= 0) and return 0.0 in that case before indexing
iv->data, keeping the existing NULL checks (iv and iv->data) and preserving the
early return behavior.
- Around line 347-349: The fprintf call in the loop uses `%d` for the loop index
`i` which is a size_t; change the format specifier to `%zu` to match `size_t`
and avoid undefined behavior, leaving the `%f` specifiers for the floating
arrays `eng`, `t_int`, and `e_int` unchanged; update the fprintf invocation in
the loop that iterates `for (size_t i = 0; eng[i] != F_END_OF_LIST; i++)`
accordingly.
- Around line 59-62: The call to rfac_mklist in rfac/rfrmin.c passes arguments
in the wrong order and includes an extra de parameter; update the call in the
#else branch so it matches rfac_mklist(eng, e_int, t_int, shift, exp_list,
exp_leng, the_list, the_leng) by removing de and passing (energies,
expt_intensities, theory_intensities, shift, experimental_data,
experimental_leng, theory_data, theory_leng) in that exact order so shift is the
4th argument and experimental_data the 5th.
- Around line 419-420: The warning call uses the wrong format specifier for
i_list (a size_t): change the "%d" to "%zu" in the WARNING_MSG invocation so it
matches i_list's type (e.g., WARNING_MSG("No overlap in IV curve No.%zu for
shift %.1f eV\n", i_list, shift);), leaving the shift format "%.1f" unchanged.
- Around line 26-27: Remove the non-portable include <malloc.h> from rfrmin.c
and rely on the standard <stdlib.h> already present for malloc/free
declarations; delete the line with #include <malloc.h> so only #include
<stdlib.h> remains to ensure portability across platforms.
- Around line 408-409: Remove the leftover debug-induced termination: in the
rfac_rmin function delete the exit(0) call that follows the
rfac_iv_print_list(eng, t_int, e_int) invocation so the loop/logic can continue;
ensure no other early-exit or debug-only calls remain in rfac_rmin and run tests
to confirm normal completion.
- Around line 72-77: The loop in rfac_ivcur_get_number_of_datasets uses a
meaningless check "&iv_cur[n] != NULL"; instead, guard the function against a
NULL input up front and then loop only on the sentinel condition. Concretely, in
rfac_ivcur_get_number_of_datasets, add a check like "if (iv_cur == NULL) return
0;" before the loop, then iterate with "for (n = 0; iv_cur[n].group_id !=
END_OF_GROUP_ID; ++n)" (i.e., remove the &iv_cur[n] != NULL condition) so the
loop terminates based on the sentinel group_id.
- Around line 142-159: The switch on r_type contains return statements in each
case (calling rfac_rp, rfac_r1, rfac_r2, rfac_rb) so the trailing break
statements are unreachable—remove those breaks; also the final "return faux;"
after the switch is unreachable because the default branch calls exit(EINVAL),
so delete that return (or alternatively change the default to return faux if you
want a non-exiting fallback). Update the switch in rfrmin.c to remove the
unreachable breaks and eliminate the redundant return faux (or adjust the
default as noted) so control flow is clean.
- Around line 448-450: The call to rfac_ivcur_write passes the wrong variables:
replace the loop-local shift and e_range with the best-shift and the correct
energy values—use *s_min_ptr for the shift argument, use args->s_step for the de
(energy step) argument, and use *e_range_ptr for the total_energy_range argument
while keeping iv_cur, n_leng, *r_min_ptr, args->r_type, and args->vi as before.
- Around line 296-298: Before computing norm, compute a separate variable for
the denominator (e.g., sum_e = rfac_iv_get_sum(e_int, n_leng)) and guard against
zero: if sum_e == 0 handle the edge case instead of dividing (options: set norm
= 0 to avoid NaN, skip the normalization step, or return an error status after
logging). Replace the direct division norm = rfac_iv_get_sum(t_int, n_leng) /
rfac_iv_get_sum(e_int, n_leng) with this guarded logic and use the same symbols
(rfac_iv_get_sum, t_int, e_int, n_leng, norm) so callers/flow remain clear.
- Around line 304-307: The filename formatting is inconsistent between the two
branches (uses "%s.0%d" when i_curve < 9 but "%s0%d" otherwise), producing
mismatched names like "prefix.09" vs "prefix010"; replace the two-branch logic
with a single consistent format that includes the dot and zero-pads the curve
number (use iv_file_prefix and i_curve+1) — e.g., use a single sprintf targeting
buffer with a format that always includes the dot and pads to two digits
(reference iv_file_prefix, i_curve, buffer).
- Line 45: These five functions (rfac_iv_get_energy_range,
rfac_ivcur_get_number_of_datasets, rfac_iv_write, rfac_ivcur_write,
rfac_iv_print_list) currently have external linkage but are only used inside
this translation unit and not declared in any public header; make them internal
by adding the static keyword to each function definition (e.g., change
rfac_iv_get_energy_range(...) to static rfac_iv_get_energy_range(...)) so they
have internal linkage, or alternatively add their declarations to the public
header rfac_func.h if they must be exported; pick the static-change approach for
minimal impact unless external use is required.
In @src/rfac/rfsort.c:
- Around line 23-28: Remove the unused stdio.h include in rfac/rfsort.c: locate
the include list at the top (the lines with #include <stdio.h>, <stddef.h>,
<stdbool.h>, <math.h> and "rfac.h") and delete the #include <stdio.h> line since
no stdio functions (e.g., printf/fprintf) are used in this file; keep the
remaining includes intact.
- Around line 30-42: The function documentation is out of sync with the
implementation: update the docblock for the sort routine in rfsort.c to list the
actual return values used by the code (specify that the function returns 0 on
success and RFAC_ALLOCATION_ERROR when passed a NULL iv pointer), and either
remove mention of RFAC_FAILURE or note it is not currently used; reference the
RFAC_ALLOCATION_ERROR constant and the function's success return value (0) so
callers know the real API contract.
- Around line 76-84: Move the initialization of iv->equidist out of the for-loop
and set it to 1 (true) before the loop, then in the loop that checks
equidistance (using cleed_real_fabs, ENG_TOLERANCE and iv->data[i].energy) as
soon as you detect a violation and set iv->equidist = false, break out of the
loop to avoid unnecessary iterations; keep all existing comparison logic intact.
- Around line 48-52: The NULL parameter check currently returns
RFAC_ALLOCATION_ERROR which is semantically incorrect; change the return value
to RFAC_INVALID_ARGUMENT for the iv == NULL check in rfsort.c (the sanity check
that returns on NULL iv) and make the equivalent change in rflorentz.c at the
analogous NULL parameter check (around line 78) so that parameter validation
uses RFAC_INVALID_ARGUMENT while RFAC_ALLOCATION_ERROR remains reserved for
actual allocation failures.
- Around line 54-74: Replace the O(n²) manual selection-sort in rfsort.c (the
nested loops that compare iv->data[i].energy and iv->data[j].energy) with a call
to qsort: implement a comparator function (e.g., rfac_iv_compare_energy) that
takes two const void* pointers, casts them to rfac_iv_data*, and compares their
energy fields, then call qsort(iv->data, iv->n_eng, sizeof(rfac_iv_data),
rfac_iv_compare_energy). Ensure the comparator returns negative/zero/positive
per C qsort conventions and remove the old nested loops.
- Line 55: Check and handle the case where iv->n_eng == 0 before any array
access: at the start of the routine that contains the loop using iv and n_eng
(the for loop starting with "for (i = 0; i< iv->n_eng-1; i++)"), add a guard
that returns or otherwise handles an empty IV curve when iv->n_eng == 0; also
replace the loop condition with a non-underflow-safe form (e.g., "i + 1 <
iv->n_eng") or rely on the earlier guard so the loop cannot underflow, and
ensure the later accesses iv->data[0] and iv->data[iv->n_eng - 1] are only
executed after confirming iv->n_eng > 0. Ensure all references to iv->data,
iv->n_eng, and the loop index i are protected by this validation.
In @src/rfac/rfspline.c:
- Line 79: The end-of-function comment is inconsistent: it reads "cr_spline" but
the function is named "rfac_iv_spline"; update the trailing comment after the
function body in rfac_iv_spline to reference "rfac_iv_spline" (or remove the
redundant name) so the comment matches the function identifier; search for and
fix any other similar mismatched end comments in rfspline.c to keep names
consistent.
In @src/rfac/rfsplint.c:
- Line 78: The end-of-function comment incorrectly names the function as
"cr_splint"; update that trailing comment to reference the actual function name
"rfac_splint" so the comment matches the function definition and avoids
confusion when scanning the code.
- Around line 33-34: The doc comment for parameter 'list' in rfsplint.c is
incorrect: it is marked as [in,out] but the function only reads from 'list' and
never modifies it; update the parameter annotation to [in] in the comment for
the function (referenced alongside cr_spline) so the documentation accurately
reflects that 'list' is input-only.
- Around line 47-52: The function rfac_splint dereferences list and uses leng to
set up a binary search (klo, khi) without validating inputs; add an early input
validation in rfac_splint that checks list != NULL and leng >= 2 and handle
invalid inputs (e.g., return a safe sentinel such as NaN or a documented error
value and/or set an error flag) instead of proceeding; ensure the binary search
initialization (klo, khi) only runs when leng >= 2 so khi cannot be negative or
zero and adjust any downstream assumptions about valid indices accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/cleed_core/CMakeLists.txtsrc/rfac/rfac.csrc/rfac/rflorentz.csrc/rfac/rfr1.csrc/rfac/rfr2.csrc/rfac/rfrb.csrc/rfac/rfrmin.csrc/rfac/rfsort.csrc/rfac/rfspline.csrc/rfac/rfsplint.ctests/test_lattice_read.ctests/test_lattice_read_edge_cases.ctests/test_lattice_setup.c
🧰 Additional context used
🧬 Code graph analysis (2)
src/rfac/rfrmin.c (7)
src/rfac/rfmklide.c (1)
rfac_mklide(63-118)src/rfac/rfmklist.c (1)
rfac_mklist(62-169)src/rfac/rfrp.c (1)
rfac_rp(44-89)src/rfac/rfr1.c (1)
rfac_r1(49-81)src/rfac/rfr2.c (1)
rfac_r2(48-95)src/rfac/rfrb.c (1)
rfac_rb(47-75)src/rfac/rfac.c (1)
rfactor_name(34-44)
src/rfac/rflorentz.c (1)
src/rfac/rfsort.c (1)
rfac_iv_sort(43-91)
🪛 Clang (14.0.6)
src/rfac/rfrb.c
[error] 22-22: 'stdio.h' file not found
(clang-diagnostic-error)
[warning] 47-47: 2 adjacent parameters of 'rfac_rb' of similar type ('const int *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 47-47: the first parameter in the range is 'eng'
(clang)
[note] 47-47: the last parameter in the range is 'e_int'
(clang)
src/rfac/rfsplint.c
[error] 20-20: 'stdlib.h' file not found
(clang-diagnostic-error)
[warning] 50-50: multiple declarations in a single statement reduces readability
(readability-isolate-declaration)
[warning] 50-50: variable 'klo' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable 'khi' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable 'k' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable name 'k' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 59-59: statement should be inside braces
(readability-braces-around-statements)
[warning] 60-60: statement should be inside braces
(readability-braces-around-statements)
src/rfac/rfsort.c
[error] 23-23: 'stdio.h' file not found
(clang-diagnostic-error)
[warning] 43-43: parameter name 'iv' is too short, expected at least 3 characters
(readability-identifier-length)
src/rfac/rfspline.c
[error] 20-20: 'stdlib.h' file not found
(clang-diagnostic-error)
[warning] 38-38: parameter name 'iv' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 41-41: variable 'k' is not initialized
(cppcoreguidelines-init-variables)
[warning] 41-41: variable name 'k' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 44-44: statement should be inside braces
(readability-braces-around-statements)
src/rfac/rfr1.c
[error] 22-22: 'stdio.h' file not found
(clang-diagnostic-error)
[warning] 49-49: 2 adjacent parameters of 'rfac_r1' of similar type ('const int *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 49-49: the first parameter in the range is 'eng'
(clang)
[note] 49-49: the last parameter in the range is 'e_int'
(clang)
[warning] 51-51: multiple declarations in a single statement reduces readability
(readability-isolate-declaration)
[warning] 51-51: variable 'i_eng' is not initialized
(cppcoreguidelines-init-variables)
[warning] 51-51: variable 'n_eng' is not initialized
(cppcoreguidelines-init-variables)
src/rfac/rfrmin.c
[error] 23-23: 'errno.h' file not found
(clang-diagnostic-error)
[warning] 47-47: statement should be inside braces
(readability-braces-around-statements)
[warning] 47-47: narrowing conversion from 'double' to 'int'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 52-52: 3 adjacent parameters of 'rfac_iv_make_list' of similar type ('int *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 52-52: the first parameter in the range is 'energies'
(clang)
[note] 53-53: the last parameter in the range is 'theory_intensities'
(clang)
[warning] 53-53: 2 adjacent parameters of 'rfac_iv_make_list' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 53-53: the first parameter in the range is 'de'
(clang)
[note] 53-53: the last parameter in the range is 'shift'
(clang)
[warning] 128-128: 3 adjacent parameters of 'rfactor_calculate' of similar type ('int *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 128-128: the first parameter in the range is 'energies'
(clang)
[note] 129-129: the last parameter in the range is 'theory_intensities'
(clang)
[warning] 130-130: 2 adjacent parameters of 'rfactor_calculate' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 130-130: the first parameter in the range is 'vi'
(clang)
[note] 130-130: the last parameter in the range is 'r_type'
(clang)
[warning] 206-206: 2 adjacent parameters of 'rfac_iv_write' of similar type ('const int *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 206-206: the first parameter in the range is 'energies'
(clang)
[note] 206-206: the last parameter in the range is 'intensities'
(clang)
[warning] 206-206: 4 adjacent parameters of 'rfac_iv_write' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 206-206: the first parameter in the range is 'n_leng'
(clang)
[note] 207-207: the last parameter in the range is 'r_type'
(clang)
[warning] 208-208: 5 adjacent parameters of 'rfac_iv_write' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 208-208: the first parameter in the range is 'weight'
(clang)
[note] 209-209: the last parameter in the range is 'norm'
(clang)
[warning] 212-212: variable 'r_name' is not initialized
(cppcoreguidelines-init-variables)
[warning] 213-213: variable 'data_type' is not initialized
(cppcoreguidelines-init-variables)
[warning] 235-235: if with identical then and else branches
(bugprone-branch-clone)
[note] 241-241: else branch starts here
(clang)
[warning] 264-264: 7 adjacent parameters of 'rfac_ivcur_write' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 264-264: the first parameter in the range is 'n_leng'
(clang)
[note] 265-265: the last parameter in the range is 'vi'
(clang)
[warning] 264-264: parameter name 'de' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 265-265: parameter name 'vi' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 267-267: variable 'filename' is not initialized
(cppcoreguidelines-init-variables)
[warning] 268-268: variable 'buffer' is not initialized
(cppcoreguidelines-init-variables)
[warning] 344-344: 2 adjacent parameters of 'rfac_iv_print_list' of similar type ('const int *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 344-344: the first parameter in the range is 'eng'
(clang)
[note] 344-344: the last parameter in the range is 't_int'
(clang)
[warning] 345-345: statement should be inside braces
(readability-braces-around-statements)
[warning] 370-370: 3 adjacent parameters of 'rfac_rmin' of similar type ('int *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 370-370: the first parameter in the range is 'iv_cur'
(clang)
[note] 371-371: the last parameter in the range is 'r_min_ptr'
(clang)
[warning] 371-371: 2 adjacent parameters of 'rfac_rmin' of similar type ('int *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 371-371: the first parameter in the range is 's_min_ptr'
(clang)
[note] 371-371: the last parameter in the range is 'e_range_ptr'
(clang)
src/rfac/rfac.c
[error] 20-20: 'stdbool.h' file not found
(clang-diagnostic-error)
[warning] 48-48: 5 adjacent parameters of 'rfactor_print' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 48-48: the first parameter in the range is 'type'
(clang)
[note] 52-52: the last parameter in the range is 'e_range'
(clang)
[warning] 50-50: parameter name 'rr' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 54-54: statement should be inside braces
(readability-braces-around-statements)
[warning] 55-55: variable 'rf' is not initialized
(cppcoreguidelines-init-variables)
[warning] 55-55: variable name 'rf' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 166-166: statement should be inside braces
(readability-braces-around-statements)
[warning] 169-169: statement should be inside braces
(readability-braces-around-statements)
[warning] 170-170: statement should be inside braces
(readability-braces-around-statements)
[warning] 199-199: statement should be inside braces
(readability-braces-around-statements)
[warning] 219-219: statement should be inside braces
(readability-braces-around-statements)
[warning] 248-248: parameter name 'iv' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 248-248: parameter name 'x' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 248-248: parameter name 'fx' is too short, expected at least 3 characters
(readability-identifier-length)
src/rfac/rflorentz.c
[error] 20-20: 'stdio.h' file not found
(clang-diagnostic-error)
[warning] 51-51: parameter name 'iv' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 51-51: parameter name 'vi' is too short, expected at least 3 characters
(readability-identifier-length)
src/rfac/rfr2.c
[error] 23-23: 'stdio.h' file not found
(clang-diagnostic-error)
[warning] 48-48: 2 adjacent parameters of 'rfac_r2' of similar type ('const int *') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 48-48: the first parameter in the range is 'eng'
(clang)
[note] 48-48: the last parameter in the range is 'e_int'
(clang)
[warning] 92-92: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.19.0)
src/rfac/rfrb.c
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingInclude)
[style] 47-47: The function 'rfac_rb' is never used.
(unusedFunction)
src/rfac/rfsplint.c
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingInclude)
[style] 47-47: The function 'rfac_splint' is never used.
(unusedFunction)
src/rfac/rfsort.c
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingInclude)
[style] 43-43: The function 'rfac_iv_sort' is never used.
(unusedFunction)
src/rfac/rfspline.c
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingInclude)
[style] 38-38: The function 'rfac_iv_spline' is never used.
(unusedFunction)
src/rfac/rfr1.c
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingInclude)
[style] 49-49: The function 'rfac_r1' is never used.
(unusedFunction)
src/rfac/rfrmin.c
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
[information] 31-31: Include file
(missingInclude)
[style] 109-109: The function 'rfac_ivcur_get_overlap' is never used.
(unusedFunction)
[style] 370-370: The function 'rfac_rmin' is never used.
(unusedFunction)
[style] 45-45: The function 'rfac_iv_get_energy_range' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
[style] 72-72: The function 'rfac_ivcur_get_number_of_datasets' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
[style] 205-205: The function 'rfac_iv_write' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
[style] 263-263: The function 'rfac_ivcur_write' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
[style] 344-344: The function 'rfac_iv_print_list' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
src/rfac/rfac.c
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingInclude)
[error] 190-190: Memory leak
(memleak)
[style] 46-46: The function 'rfactor_print' is never used.
(unusedFunction)
[style] 71-71: The function 'rfac_iv_init' is never used.
(unusedFunction)
[style] 94-94: The function 'rfac_iv_alloc' is never used.
(unusedFunction)
[style] 118-118: The function 'rfac_args_init' is never used.
(unusedFunction)
[style] 164-164: The function 'rfac_iv_copy' is never used.
(unusedFunction)
[style] 215-215: The function 'rfac_ivcur_free_all' is never used.
(unusedFunction)
[style] 248-248: The function 'rfac_iv_to_arrays' is never used.
(unusedFunction)
[style] 34-34: The function 'rfactor_name' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
[style] 145-145: The function 'rfac_iv_free' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
[style] 197-197: The function 'rfac_ivcur_free' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
src/rfac/rflorentz.c
[information] 20-20: Include file
(missingIncludeSystem)
[information] 21-21: Include file
(missingIncludeSystem)
[information] 22-22: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingInclude)
[style] 51-51: The function 'rfac_iv_lorentz_smooth' is never used.
(unusedFunction)
src/rfac/rfr2.c
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 26-26: Include file
(missingInclude)
[style] 48-48: The function 'rfac_r2' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: windows-latest
🔇 Additional comments (14)
tests/test_lattice_read_edge_cases.c (2)
130-131: LGTM: Stream initialization occurs before test execution.The assignment of
stderrto both global streams inmain()before any test functions are called ensures proper initialization and isolation for test diagnostics.
13-13: No action needed—the extern declarations are properly visible.The extern declarations for
inf_streamandctr_streamare present insrc/include/lattice.h(lines 88–89), which is included by this file. The comment on line 13 accurately documents that the definitions are inlatt_streams.c, and the assignments on lines 130–131 have full visibility of the extern declarations through the included header. No implicit declaration warnings or linkage errors would occur.Likely an incorrect or invalid review comment.
tests/test_lattice_read.c (2)
13-13: Verify extern declarations are visible (same concern as test_lattice_read_edge_cases.c).This file has the same pattern as
test_lattice_read_edge_cases.c- it relies on external linkage forinf_streamandctr_streamwithout explicit extern declarations visible in this file. Please refer to the verification script in my review oftest_lattice_read_edge_cases.cto confirm the extern declarations exist in the included headers.
180-181: LGTM: Proper stream initialization.Stream initialization before test execution is correct and consistent with the other test files.
tests/test_lattice_setup.c (2)
13-13: Verify extern declarations (consistent with other test files).This file follows the same pattern as
test_lattice_read_edge_cases.candtest_lattice_read.c. Please ensure extern declarations forinf_streamandctr_streamare visible in the included headers (see verification script in the review oftest_lattice_read_edge_cases.c).
90-91: LGTM: Consistent initialization pattern.Stream initialization is correct and consistent with the other test files in this PR.
src/cleed_core/CMakeLists.txt (5)
1-6: LGTM!Source file list is clean and follows CMake conventions.
8-9: LGTM!Creating both shared and static library targets from the same sources is a reasonable pattern for providing flexibility to consumers.
11-13: LGTM!Setting the same output name for the static library on non-Windows platforms is appropriate since the file extensions (
.so/.dylibvs.a) distinguish the library types.
41-41: LGTM!Static library installation rule is correct.
18-26: Windows workaround is necessary; consider tracking the underlying symbol export issue.The comment correctly identifies a real issue: without
__declspec(dllexport)declarations, the shared libraries (search, rfac) don't generate proper import libraries (.lib) on Windows. The workaround of linking the shared library against static libraries is pragmatic but not ideal.Linking a SHARED library against STATIC libraries on Windows can lead to increased binary size and potential symbol visibility complications. This pattern works around a deeper problem: the search and rfac libraries lack proper Windows symbol exports.
Consider adding a TODO or tracking issue to implement proper symbol export macros in the search and rfac libraries (e.g., using
__declspec(dllexport)on Windows or__attribute__((visibility))for consistent cross-platform exports), which would allow the shared libraries to be properly consumed without this workaround.src/rfac/rfr2.c (1)
87-91: Review negative check logic for sum of squares.The check
if (norm_sum < 0.)guards against a mathematically impossible condition—norm_sumis a sum of squared differences (line 84) and cannot be negative. This suggests either:
- A logic error (checking the wrong condition)
- Defensive programming against floating-point underflow (though the message says "negative", not "non-positive")
- Copy-paste from code that checked a different variable
Consider changing to
<= 0.for consistency with division-by-zero protection, or clarifying the intent with a comment.src/rfac/rfrmin.c (2)
169-184: LGTM!The implementation is straightforward and correct. The DEBUG-guarded NULL check is appropriate.
238-244: Format specifier mismatch forsize_tloop variable.Lines 238 and 243 use
size_t ias the loop variable, but the format specifier issue is not present here (the loop doesn't printi). However, the logic is correct. The static analysis warning about "identical branches" is a false positive — line 239 writesintensities[i]while line 244 writesintensities[i]*norm.
| target_include_directories(cleedCore PUBLIC ${PROJECT_SOURCE_DIR}/src/include) | ||
| target_include_directories(cleedCoreStatic PUBLIC ${PROJECT_SOURCE_DIR}/src/include) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reducing duplication with a loop or INTERFACE library.
Both targets share the same include directories. You could reduce duplication:
♻️ Optional: Use a loop to reduce duplication
-target_include_directories(cleedCore PUBLIC ${PROJECT_SOURCE_DIR}/src/include)
-target_include_directories(cleedCoreStatic PUBLIC ${PROJECT_SOURCE_DIR}/src/include)
+foreach(target cleedCore cleedCoreStatic)
+ target_include_directories(${target} PUBLIC ${PROJECT_SOURCE_DIR}/src/include)
+endforeach()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_include_directories(cleedCore PUBLIC ${PROJECT_SOURCE_DIR}/src/include) | |
| target_include_directories(cleedCoreStatic PUBLIC ${PROJECT_SOURCE_DIR}/src/include) | |
| foreach(target cleedCore cleedCoreStatic) | |
| target_include_directories(${target} PUBLIC ${PROJECT_SOURCE_DIR}/src/include) | |
| endforeach() |
| if (WIN32) | ||
| install(TARGETS cleedCore | ||
| COMPONENT runtime | ||
| RUNTIME DESTINATION bin | ||
| LIBRARY DESTINATION bin | ||
| ) | ||
| else() | ||
| install(TARGETS cleedCore | ||
| COMPONENT runtime | ||
| LIBRARY DESTINATION lib | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
Missing ARCHIVE destination on Windows may prevent consumers from linking.
On Windows, CMake's ARCHIVE destination is where the import library (.lib) is installed. Without it, consumers won't be able to link against cleedCore.dll at build time.
If you intend for consumers to link against the shared library on Windows, add the ARCHIVE destination:
🔧 Proposed fix
if (WIN32)
install(TARGETS cleedCore
COMPONENT runtime
RUNTIME DESTINATION bin
LIBRARY DESTINATION bin
+ ARCHIVE DESTINATION lib
)
else()If the shared library is only intended for runtime use (not for linking), you can ignore this. However, this conflicts with the current workaround of linking cleedCore statically on Windows anyway.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (WIN32) | |
| install(TARGETS cleedCore | |
| COMPONENT runtime | |
| RUNTIME DESTINATION bin | |
| LIBRARY DESTINATION bin | |
| ) | |
| else() | |
| install(TARGETS cleedCore | |
| COMPONENT runtime | |
| LIBRARY DESTINATION lib | |
| ) | |
| endif() | |
| if (WIN32) | |
| install(TARGETS cleedCore | |
| COMPONENT runtime | |
| RUNTIME DESTINATION bin | |
| LIBRARY DESTINATION bin | |
| ARCHIVE DESTINATION lib | |
| ) | |
| else() | |
| install(TARGETS cleedCore | |
| COMPONENT runtime | |
| LIBRARY DESTINATION lib | |
| ) | |
| endif() |
🤖 Prompt for AI Agents
In @src/cleed_core/CMakeLists.txt around lines 28 - 39, The Windows install
block for the cleedCore target omits an ARCHIVE destination so the import
library (.lib) won't be installed; update the WIN32 install(TARGETS cleedCore
...) invocation to include "ARCHIVE DESTINATION bin" alongside the existing
RUNTIME and LIBRARY DESTINATION bin entries so the import library is installed
for consumers to link against the DLL (i.e., add ARCHIVE DESTINATION bin in the
WIN32 branch of the install call for cleedCore).
| void rfac_iv_free(rfac_iv *_iv) | ||
| { | ||
| if (_iv != NULL) | ||
| { | ||
| if (_iv->data != NULL) free(_iv->data); | ||
| _iv->data = NULL; | ||
| free(_iv); | ||
| _iv = NULL; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Ineffective NULL assignment on local pointer.
Setting _iv = NULL at line 152 has no effect on the caller's pointer since _iv is passed by value. The caller's pointer will remain a dangling pointer after this call. Consider using a double pointer if you want to NULL-out the caller's pointer, or simply remove the misleading assignment.
Option 1: Remove misleading assignment
void rfac_iv_free(rfac_iv *_iv)
{
if (_iv != NULL)
{
if (_iv->data != NULL) free(_iv->data);
- _iv->data = NULL;
free(_iv);
- _iv = NULL;
}
}Option 2: Use double pointer to NULL caller's pointer
-void rfac_iv_free(rfac_iv *_iv)
+void rfac_iv_free(rfac_iv **_iv)
{
- if (_iv != NULL)
+ if (_iv != NULL && *_iv != NULL)
{
- if (_iv->data != NULL) free(_iv->data);
- _iv->data = NULL;
- free(_iv);
- _iv = NULL;
+ if ((*_iv)->data != NULL) free((*_iv)->data);
+ free(*_iv);
+ *_iv = NULL;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void rfac_iv_free(rfac_iv *_iv) | |
| { | |
| if (_iv != NULL) | |
| { | |
| if (_iv->data != NULL) free(_iv->data); | |
| _iv->data = NULL; | |
| free(_iv); | |
| _iv = NULL; | |
| } | |
| } | |
| void rfac_iv_free(rfac_iv *_iv) | |
| { | |
| if (_iv != NULL) | |
| { | |
| if (_iv->data != NULL) free(_iv->data); | |
| free(_iv); | |
| } | |
| } |
🧰 Tools
🪛 Cppcheck (2.19.0)
[style] 145-145: The function 'rfac_iv_free' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
🤖 Prompt for AI Agents
In @src/rfac/rfac.c around lines 145 - 154, The function rfac_iv_free currently
sets the local parameter _iv to NULL after freeing, which does not affect the
caller and is misleading; either remove the pointless assignment "_iv = NULL"
from rfac_iv_free, or change the API to accept a double pointer (rfac_iv **_iv)
so you free(*_iv), set * _iv = NULL, and update all callers to pass the address
of their pointer. Ensure the chosen approach updates function signature and call
sites consistently if you pick the double-pointer option.
| void rfac_iv_copy(rfac_iv *dest, const rfac_iv *src) | ||
| { | ||
| if (src == NULL) return; /* nothing to do */ | ||
|
|
||
| /* allocate memory for dest if needed */ | ||
| if (dest == NULL) | ||
| if ( ( dest = (rfac_iv *) malloc(sizeof(rfac_iv)) ) == NULL) | ||
| return; | ||
|
|
||
| /* free data is exists; to be allocated new memory later */ | ||
| if (dest->data != NULL) free(dest->data); | ||
|
|
||
| /* copy member contents */ | ||
| memcpy(dest, src, sizeof(rfac_iv)); | ||
|
|
||
| /* allocate memory for new data */ | ||
| dest->data = (rfac_iv_data *) malloc(dest->n_eng*sizeof(rfac_iv_data)); | ||
|
|
||
| if (dest->data == NULL) | ||
| { | ||
| free(dest); | ||
| return; | ||
| } | ||
|
|
||
| /* copy data */ | ||
| memcpy(dest->data, src->data, dest->n_eng*sizeof(rfac_iv_data)); | ||
| } |
There was a problem hiding this comment.
Critical: Memory leak and undefined behavior when dest is NULL.
Two serious issues:
-
Memory leak: When
destis NULL, the function allocates memory (line 170) but assigns it to the local variabledest. The caller never receives this pointer, causing a memory leak (confirmed by static analysis). -
Undefined behavior: After allocating
destat line 170, line 174 readsdest->datafrom uninitialized memory (malloc doesn't zero-initialize), potentially freeing an arbitrary pointer.
🐛 Recommended fix: Return the pointer and fix initialization
-void rfac_iv_copy(rfac_iv *dest, const rfac_iv *src)
+rfac_iv *rfac_iv_copy(rfac_iv *dest, const rfac_iv *src)
{
- if (src == NULL) return; /* nothing to do */
+ if (src == NULL) return dest; /* nothing to do */
/* allocate memory for dest if needed */
if (dest == NULL)
- if ( ( dest = (rfac_iv *) malloc(sizeof(rfac_iv)) ) == NULL)
- return;
-
- /* free data is exists; to be allocated new memory later */
- if (dest->data != NULL) free(dest->data);
+ {
+ if ( ( dest = (rfac_iv *) malloc(sizeof(rfac_iv)) ) == NULL)
+ return NULL;
+ dest->data = NULL; /* initialize to avoid UB below */
+ }
+ else
+ {
+ /* free data if exists; to be allocated new memory later */
+ if (dest->data != NULL) free(dest->data);
+ }
/* copy member contents */
memcpy(dest, src, sizeof(rfac_iv));
/* allocate memory for new data */
dest->data = (rfac_iv_data *) malloc(dest->n_eng*sizeof(rfac_iv_data));
if (dest->data == NULL)
{
free(dest);
- return;
+ return NULL;
}
/* copy data */
memcpy(dest->data, src->data, dest->n_eng*sizeof(rfac_iv_data));
+
+ return dest;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void rfac_iv_copy(rfac_iv *dest, const rfac_iv *src) | |
| { | |
| if (src == NULL) return; /* nothing to do */ | |
| /* allocate memory for dest if needed */ | |
| if (dest == NULL) | |
| if ( ( dest = (rfac_iv *) malloc(sizeof(rfac_iv)) ) == NULL) | |
| return; | |
| /* free data is exists; to be allocated new memory later */ | |
| if (dest->data != NULL) free(dest->data); | |
| /* copy member contents */ | |
| memcpy(dest, src, sizeof(rfac_iv)); | |
| /* allocate memory for new data */ | |
| dest->data = (rfac_iv_data *) malloc(dest->n_eng*sizeof(rfac_iv_data)); | |
| if (dest->data == NULL) | |
| { | |
| free(dest); | |
| return; | |
| } | |
| /* copy data */ | |
| memcpy(dest->data, src->data, dest->n_eng*sizeof(rfac_iv_data)); | |
| } | |
| rfac_iv *rfac_iv_copy(rfac_iv *dest, const rfac_iv *src) | |
| { | |
| if (src == NULL) return dest; /* nothing to do */ | |
| /* allocate memory for dest if needed */ | |
| if (dest == NULL) | |
| { | |
| if ( ( dest = (rfac_iv *) malloc(sizeof(rfac_iv)) ) == NULL) | |
| return NULL; | |
| dest->data = NULL; /* initialize to avoid UB below */ | |
| } | |
| else | |
| { | |
| /* free data if exists; to be allocated new memory later */ | |
| if (dest->data != NULL) free(dest->data); | |
| } | |
| /* copy member contents */ | |
| memcpy(dest, src, sizeof(rfac_iv)); | |
| /* allocate memory for new data */ | |
| dest->data = (rfac_iv_data *) malloc(dest->n_eng*sizeof(rfac_iv_data)); | |
| if (dest->data == NULL) | |
| { | |
| free(dest); | |
| return NULL; | |
| } | |
| /* copy data */ | |
| memcpy(dest->data, src->data, dest->n_eng*sizeof(rfac_iv_data)); | |
| return dest; | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 166-166: statement should be inside braces
(readability-braces-around-statements)
[warning] 169-169: statement should be inside braces
(readability-braces-around-statements)
[warning] 170-170: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.19.0)
[error] 190-190: Memory leak
(memleak)
[style] 164-164: The function 'rfac_iv_copy' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In @src/rfac/rfac.c around lines 164 - 190, The rfac_iv_copy function leaks
memory and invokes undefined behavior when dest is NULL because it allocates
into the local dest and then reads dest->data uninitialized; fix by changing the
API or initialization: either (preferred) change rfac_iv_copy to return rfac_iv*
(or provide a separate rfac_iv_clone function) so it can allocate a new rfac_iv
for the caller when dest is NULL, or require dest non-NULL and assert/return
error; when allocating a new rfac_iv ensure you zero-initialize it (use calloc
or memset) so dest->data is NULL before any free, copy the struct fields from
src into dest except the data pointer, then allocate dest->data based on
src->n_eng and handle allocation failure by freeing any allocated memory and
returning NULL (or an error), and finally memcpy the data from src->data to
dest->data; refer to function name rfac_iv_copy and members dest->data and
dest->n_eng when making these changes.
| void rfac_ivcur_free_all(rfac_ivcur *ivcur) | ||
| { | ||
| size_t n=0, i; | ||
|
|
||
| if (ivcur == NULL) return; | ||
| while(ivcur->group_id != END_OF_GROUP_ID) | ||
| { | ||
| n++; | ||
| } | ||
| for (i=n; i > 0; i--) /* free curves in reverse order - possibly safer? */ | ||
| rfac_ivcur_free(&ivcur[i]); | ||
| free(ivcur); | ||
| } |
There was a problem hiding this comment.
Critical: Infinite loop due to missing pointer advancement.
The while loop at lines 220-223 never advances the ivcur pointer, causing an infinite loop if ivcur[0].group_id != END_OF_GROUP_ID.
Additionally, line 225 passes &ivcur[i] to rfac_ivcur_free, which calls free() on array elements. If ivcur was allocated as a contiguous array, you cannot free individual elements—only the base pointer should be freed. The nested experimental and theory members should be freed, but not the ivcur elements themselves.
🐛 Proposed fix
void rfac_ivcur_free_all(rfac_ivcur *ivcur)
{
size_t n=0, i;
if (ivcur == NULL) return;
- while(ivcur->group_id != END_OF_GROUP_ID)
+ while(ivcur[n].group_id != END_OF_GROUP_ID)
{
n++;
}
- for (i=n; i > 0; i--) /* free curves in reverse order - possibly safer? */
- rfac_ivcur_free(&ivcur[i]);
+ /* free nested IV curves (but not the ivcur elements themselves) */
+ for (i = 0; i < n; i++)
+ {
+ rfac_iv_free(ivcur[i].experimental);
+ rfac_iv_free(ivcur[i].theory);
+ }
+ /* free the array itself */
free(ivcur);
}🧰 Tools
🪛 Clang (14.0.6)
[warning] 219-219: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.19.0)
[style] 215-215: The function 'rfac_ivcur_free_all' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In @src/rfac/rfac.c around lines 215 - 227, The function rfac_ivcur_free_all
currently never advances the pointer and wrongly calls rfac_ivcur_free on each
array element (which itself calls free on the element). Fix by counting elements
with an index: loop while ivcur[i].group_id != END_OF_GROUP_ID incrementing i to
get n; then iterate i from n-1 down to 0 and free only the per-element heap
members (e.g. free ivcur[i].experimental, ivcur[i].theory, and any other
allocated members) rather than freeing &ivcur[i]; finally free the base ivcur
pointer. If rfac_ivcur_free must be reused, change or add a helper (e.g.
rfac_ivcur_free_members) that frees the nested members but does not free the
struct itself, and call that from rfac_ivcur_free_all.
| /* check equidistance */ | ||
| for (i = 1, iv->equidist = 1; i< iv->n_eng-1; i++) | ||
| { | ||
| if (cleed_real_fabs ((2*iv->data[i].energy - | ||
| iv->data[i+1].energy - iv->data[i-1].energy) ) > ENG_TOLERANCE ) | ||
| { | ||
| iv->equidist = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider short-circuit optimization for equidistance check.
The equidistance check continues iterating even after finding the first violation. While correct, you could break early once iv->equidist is set to false for a minor performance improvement.
Also, initializing iv->equidist in the for-loop initializer (line 77) is valid C but unconventional. Consider moving it to a separate statement for clarity.
♻️ Optional refactor
/* check equidistance */
- for (i = 1, iv->equidist = 1; i< iv->n_eng-1; i++)
+ iv->equidist = true;
+ for (i = 1; i < iv->n_eng-1; i++)
{
if (cleed_real_fabs ((2*iv->data[i].energy -
iv->data[i+1].energy - iv->data[i-1].energy) ) > ENG_TOLERANCE )
{
iv->equidist = false;
+ break;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* check equidistance */ | |
| for (i = 1, iv->equidist = 1; i< iv->n_eng-1; i++) | |
| { | |
| if (cleed_real_fabs ((2*iv->data[i].energy - | |
| iv->data[i+1].energy - iv->data[i-1].energy) ) > ENG_TOLERANCE ) | |
| { | |
| iv->equidist = false; | |
| } | |
| } | |
| /* check equidistance */ | |
| iv->equidist = true; | |
| for (i = 1; i < iv->n_eng-1; i++) | |
| { | |
| if (cleed_real_fabs ((2*iv->data[i].energy - | |
| iv->data[i+1].energy - iv->data[i-1].energy) ) > ENG_TOLERANCE ) | |
| { | |
| iv->equidist = false; | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/rfac/rfsort.c around lines 76 - 84, Move the initialization of
iv->equidist out of the for-loop and set it to 1 (true) before the loop, then in
the loop that checks equidistance (using cleed_real_fabs, ENG_TOLERANCE and
iv->data[i].energy) as soon as you detect a violation and set iv->equidist =
false, break out of the loop to avoid unnecessary iterations; keep all existing
comparison logic intact.
|
|
||
| free(buf); | ||
|
|
||
| } /* end of function cr_spline */ |
There was a problem hiding this comment.
Fix inconsistent function name in comment.
The end comment references "cr_spline" but the function is named "rfac_iv_spline".
📝 Proposed fix
-} /* end of function cr_spline */
+} /* end of function rfac_iv_spline */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } /* end of function cr_spline */ | |
| } /* end of function rfac_iv_spline */ |
🤖 Prompt for AI Agents
In @src/rfac/rfspline.c at line 79, The end-of-function comment is inconsistent:
it reads "cr_spline" but the function is named "rfac_iv_spline"; update the
trailing comment after the function body in rfac_iv_spline to reference
"rfac_iv_spline" (or remove the redundant name) so the comment matches the
function identifier; search for and fix any other similar mismatched end
comments in rfspline.c to keep names consistent.
| * \param[in,out] list pointer to array of energy/intensity/deriv2 values, | ||
| * the latter of which is generated in function cr_spline. |
There was a problem hiding this comment.
Correct the documentation parameter direction.
The documentation states list is [in,out], but the function only reads from list and never modifies it. The correct annotation should be [in].
📝 Proposed fix
- * \param[in,out] list pointer to array of energy/intensity/deriv2 values,
+ * \param[in] list pointer to array of energy/intensity/deriv2 values,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * \param[in,out] list pointer to array of energy/intensity/deriv2 values, | |
| * the latter of which is generated in function cr_spline. | |
| * \param[in] list pointer to array of energy/intensity/deriv2 values, | |
| * the latter of which is generated in function cr_spline. |
🤖 Prompt for AI Agents
In @src/rfac/rfsplint.c around lines 33 - 34, The doc comment for parameter
'list' in rfsplint.c is incorrect: it is marked as [in,out] but the function
only reads from 'list' and never modifies it; update the parameter annotation to
[in] in the comment for the function (referenced alongside cr_spline) so the
documentation accurately reflects that 'list' is input-only.
| real rfac_splint(real eng, rfac_iv_data *list, size_t leng) | ||
| { | ||
| real y; | ||
| int klo, khi, k; | ||
| real h, b, a; | ||
|
|
There was a problem hiding this comment.
Add input validation for list and leng parameters.
The function dereferences list without checking for NULL and performs a binary search without validating that leng >= 2. If leng is 0 or 1, khi at line 55 could be negative or zero, causing the binary search to fail or produce undefined behavior.
🛡️ Proposed fix
real rfac_splint(real eng, rfac_iv_data *list, size_t leng)
{
real y;
int klo, khi, k;
real h, b, a;
+
+ if (list == NULL || leng < 2)
+ {
+ ERROR_MSG("Invalid input: list is NULL or leng < 2\n");
+ exit(RFAC_BAD_IV_DATA);
+ }
/* Find the right place in list by means of bisection. */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| real rfac_splint(real eng, rfac_iv_data *list, size_t leng) | |
| { | |
| real y; | |
| int klo, khi, k; | |
| real h, b, a; | |
| real rfac_splint(real eng, rfac_iv_data *list, size_t leng) | |
| { | |
| real y; | |
| int klo, khi, k; | |
| real h, b, a; | |
| if (list == NULL || leng < 2) | |
| { | |
| ERROR_MSG("Invalid input: list is NULL or leng < 2\n"); | |
| exit(RFAC_BAD_IV_DATA); | |
| } | |
| /* Find the right place in list by means of bisection. */ |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 50-50: multiple declarations in a single statement reduces readability
(readability-isolate-declaration)
[warning] 50-50: variable 'klo' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable 'khi' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable 'k' is not initialized
(cppcoreguidelines-init-variables)
[warning] 50-50: variable name 'k' is too short, expected at least 3 characters
(readability-identifier-length)
🪛 Cppcheck (2.19.0)
[style] 47-47: The function 'rfac_splint' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In @src/rfac/rfsplint.c around lines 47 - 52, The function rfac_splint
dereferences list and uses leng to set up a binary search (klo, khi) without
validating inputs; add an early input validation in rfac_splint that checks list
!= NULL and leng >= 2 and handle invalid inputs (e.g., return a safe sentinel
such as NaN or a documented error value and/or set an error flag) instead of
proceeding; ensure the binary search initialization (klo, khi) only runs when
leng >= 2 so khi cannot be negative or zero and adjust any downstream
assumptions about valid indices accordingly.
| ( (a*a*a-a)*list[klo].deriv2 + (b*b*b-b)*list[khi].deriv2 )*(h*h)/6.0); | ||
|
|
||
| return(y); | ||
| } /* end of function cr_splint */ |
There was a problem hiding this comment.
Fix inconsistent function name in comment.
The end comment references "cr_splint" but the function is named "rfac_splint".
📝 Proposed fix
-} /* end of function cr_splint */
+} /* end of function rfac_splint */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } /* end of function cr_splint */ | |
| } /* end of function rfac_splint */ |
🤖 Prompt for AI Agents
In @src/rfac/rfsplint.c at line 78, The end-of-function comment incorrectly
names the function as "cr_splint"; update that trailing comment to reference the
actual function name "rfac_splint" so the comment matches the function
definition and avoids confusion when scanning the code.
| * \note Returns zero if either \p iv or \c data member are \c NULL | ||
| * (or \c n_eng data member is 0). | ||
| */ | ||
| real rfac_iv_get_energy_range(const rfac_iv *iv) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Step 1: Check if functions are declared in rfac.h
echo "=== Checking rfac.h for function declarations ==="
rg -n "rfac_iv_get_energy_range|rfac_ivcur_get_number_of_datasets|rfac_iv_write|rfac_ivcur_write|rfac_iv_print_list" --type=c -g '*.h'Repository: Liam-Deacon/CLEED
Length of output: 110
🏁 Script executed:
# Step 2: Check rfrmin.c for these function definitions
echo "=== Checking rfrmin.c for function definitions ==="
rg -n "^(real|int|void)\s+(rfac_iv_get_energy_range|rfac_ivcur_get_number_of_datasets|rfac_iv_write|rfac_ivcur_write|rfac_iv_print_list)" src/rfac/rfrmin.cRepository: Liam-Deacon/CLEED
Length of output: 348
🏁 Script executed:
# Step 3: Search for usage of these functions across the entire codebase (excluding the definition file)
echo "=== Checking usage of these functions across codebase ==="
for func in "rfac_iv_get_energy_range" "rfac_ivcur_get_number_of_datasets" "rfac_iv_write" "rfac_ivcur_write" "rfac_iv_print_list"; do
echo "--- Usage of $func ---"
rg "$func\s*\(" --type=c | grep -v "src/rfac/rfrmin.c" | head -20
doneRepository: Liam-Deacon/CLEED
Length of output: 312
🏁 Script executed:
# Step 4: Examine the function definitions in rfrmin.c to see if they have static keyword
echo "=== Checking function definitions with context (10 lines after) ==="
for func in "rfac_iv_get_energy_range" "rfac_ivcur_get_number_of_datasets" "rfac_iv_write" "rfac_ivcur_write" "rfac_iv_print_list"; do
echo "--- Definition of $func ---"
rg -A 10 "^(real|int|void)\s+$func" src/rfac/rfrmin.c | head -20
doneRepository: Liam-Deacon/CLEED
Length of output: 1986
🏁 Script executed:
# Step 5: Check the entire rfac.h file to understand what is exported
echo "=== Listing all exported functions in rfac.h ==="
rg "^\s*(EXTERN\s+)?extern\s+" src/rfac/rfac.h | head -30Repository: Liam-Deacon/CLEED
Length of output: 182
🏁 Script executed:
# Find header files in the repository
echo "=== Finding header files ==="
find . -name "*.h" -path "*/rfac/*" | head -20Repository: Liam-Deacon/CLEED
Length of output: 89
🏁 Script executed:
# Search for rfac_ivcur_get_number_of_datasets definition
echo "=== Searching for rfac_ivcur_get_number_of_datasets ==="
rg "rfac_ivcur_get_number_of_datasets" --type=c -B 2 -A 5Repository: Liam-Deacon/CLEED
Length of output: 1268
🏁 Script executed:
# Get the exact lines mentioned in the review to see what's at lines 45, 72, 205, 263, 344
echo "=== Checking exact line numbers in rfrmin.c ==="
sed -n '45p; 72p; 205p; 263p; 344p' src/rfac/rfrmin.cRepository: Liam-Deacon/CLEED
Length of output: 397
🏁 Script executed:
# Check what header exports are actually used
echo "=== Searching for header files ==="
find . -type f -name "rfac*.h" | head -20Repository: Liam-Deacon/CLEED
Length of output: 192
🏁 Script executed:
# Search what functions are declared/exported as public API
echo "=== Checking for function declarations in all headers ==="
rg "rfac_iv_get_energy_range|rfac_ivcur_get_number_of_datasets|rfac_iv_write|rfac_ivcur_write|rfac_iv_print_list" --type=c -g '*.h' -B 1 -A 1Repository: Liam-Deacon/CLEED
Length of output: 118
🏁 Script executed:
# Check rfac.h for function declarations
echo "=== Checking ./src/include/rfac.h ==="
rg "rfac_iv_get_energy_range|rfac_ivcur_get_number_of_datasets|rfac_iv_write|rfac_ivcur_write|rfac_iv_print_list" ./src/include/rfac.h -B 1 -A 1Repository: Liam-Deacon/CLEED
Length of output: 98
🏁 Script executed:
# Check rfac_func.h for function declarations
echo "=== Checking ./src/include/rfac_func.h ==="
rg "rfac_iv_get_energy_range|rfac_ivcur_get_number_of_datasets|rfac_iv_write|rfac_ivcur_write|rfac_iv_print_list" ./src/include/rfac_func.h -B 1 -A 1Repository: Liam-Deacon/CLEED
Length of output: 103
🏁 Script executed:
# Check rfac_def.h for function declarations
echo "=== Checking ./src/include/rfac_def.h ==="
rg "rfac_iv_get_energy_range|rfac_ivcur_get_number_of_datasets|rfac_iv_write|rfac_ivcur_write|rfac_iv_print_list" ./src/include/rfac_def.h -B 1 -A 1Repository: Liam-Deacon/CLEED
Length of output: 102
🏁 Script executed:
# Get list of all exported functions from headers to understand API
echo "=== List of all rfac_iv and rfac_ivcur functions in headers ==="
rg "rfac_iv[a-z_]*\(" ./src/include/*.h | grep -E "^./src/include/rfac[^:]*:" | head -30Repository: Liam-Deacon/CLEED
Length of output: 1248
🏁 Script executed:
# Search for any usage of these functions in other files (not rfrmin.c)
echo "=== Searching for external usage across entire codebase ==="
rg "rfac_iv_get_energy_range|rfac_ivcur_get_number_of_datasets|rfac_iv_write|rfac_ivcur_write|rfac_iv_print_list" --type=c --type=h -l | grep -v rfrmin.cRepository: Liam-Deacon/CLEED
Length of output: 120
Make internal helper functions static or export them in a public header.
All five functions (rfac_iv_get_energy_range, rfac_ivcur_get_number_of_datasets, rfac_iv_write, rfac_ivcur_write, rfac_iv_print_list) have external linkage but are not used outside this translation unit and not declared in any public header. For consistency with the rest of the API (where similar functions are properly exported in rfac_func.h), either mark them static to limit scope or add declarations to the appropriate header.
🧰 Tools
🪛 Cppcheck (2.19.0)
[style] 45-45: The function 'rfac_iv_get_energy_range' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
🤖 Prompt for AI Agents
In @src/rfac/rfrmin.c at line 45, These five functions
(rfac_iv_get_energy_range, rfac_ivcur_get_number_of_datasets, rfac_iv_write,
rfac_ivcur_write, rfac_iv_print_list) currently have external linkage but are
only used inside this translation unit and not declared in any public header;
make them internal by adding the static keyword to each function definition
(e.g., change rfac_iv_get_energy_range(...) to static
rfac_iv_get_energy_range(...)) so they have internal linkage, or alternatively
add their declarations to the public header rfac_func.h if they must be
exported; pick the static-change approach for minimal impact unless external use
is required.
| fprintf(stderr, "i=%i\n", i); | ||
| #if SHIFT_DE | ||
| e_range = rfac_iv_get_energy_range(iv_cur[i].experimental); | ||
| n_leng = MAX(n_leng, (size_t)abs((int)(e_range/de))); | ||
| e_range = rfac_iv_get_energy_range(iv_cur[i].theory); | ||
| n_leng = MAX(n_leng, (size_t)abs((int)(e_range/de))); |
There was a problem hiding this comment.
Debug fprintf left in production code and potential integer overflow.
-
Line 88: The
fprintf(stderr, "i=%i\n", i)appears to be debug output that should be removed or wrapped in#if DEBUG. -
Lines 91/93: Casting to
intbeforeabs()risks overflow if the energy range divided bydeexceedsINT_MAX. Consider usingfabs()on the floating-point result instead.
Suggested fix
for(size_t i=0;
#if DEBUG
&iv_cur[i] != NULL &&
#endif
iv_cur[i].group_id != END_OF_GROUP_ID; i++)
{
- fprintf(stderr, "i=%i\n", i);
+ CONTROL_MSG(CONTROL, "i=%zu\n", i);
#if SHIFT_DE
e_range = rfac_iv_get_energy_range(iv_cur[i].experimental);
- n_leng = MAX(n_leng, (size_t)abs((int)(e_range/de)));
+ n_leng = MAX(n_leng, (size_t)fabs(e_range/de));
e_range = rfac_iv_get_energy_range(iv_cur[i].theory);
- n_leng = MAX(n_leng, (size_t)abs((int)(e_range/de)));
+ n_leng = MAX(n_leng, (size_t)fabs(e_range/de));
#else|
|
||
| /* calculate normalisation factor */ | ||
| norm = rfac_iv_get_sum(t_int, n_leng) / rfac_iv_get_sum(e_int, n_leng); |
There was a problem hiding this comment.
Potential division by zero in normalization calculation.
If rfac_iv_get_sum(e_int, n_leng) returns zero (e.g., all experimental intensities are zero), line 298 will cause a division by zero. Add a guard to handle this edge case.
Suggested fix
/* calculate normalisation factor */
- norm = rfac_iv_get_sum(t_int, n_leng) / rfac_iv_get_sum(e_int, n_leng);
+ real e_sum = rfac_iv_get_sum(e_int, n_leng);
+ if (IS_EQUAL_REAL(e_sum, 0.)) {
+ WARNING_MSG("Experimental intensities sum to zero for curve %zu\n", i_curve);
+ continue;
+ }
+ norm = rfac_iv_get_sum(t_int, n_leng) / e_sum;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* calculate normalisation factor */ | |
| norm = rfac_iv_get_sum(t_int, n_leng) / rfac_iv_get_sum(e_int, n_leng); | |
| /* calculate normalisation factor */ | |
| real e_sum = rfac_iv_get_sum(e_int, n_leng); | |
| if (IS_EQUAL_REAL(e_sum, 0.)) { | |
| WARNING_MSG("Experimental intensities sum to zero for curve %zu\n", i_curve); | |
| continue; | |
| } | |
| norm = rfac_iv_get_sum(t_int, n_leng) / e_sum; |
🤖 Prompt for AI Agents
In @src/rfac/rfrmin.c around lines 296 - 298, Before computing norm, compute a
separate variable for the denominator (e.g., sum_e = rfac_iv_get_sum(e_int,
n_leng)) and guard against zero: if sum_e == 0 handle the edge case instead of
dividing (options: set norm = 0 to avoid NaN, skip the normalization step, or
return an error status after logging). Replace the direct division norm =
rfac_iv_get_sum(t_int, n_leng) / rfac_iv_get_sum(e_int, n_leng) with this
guarded logic and use the same symbols (rfac_iv_get_sum, t_int, e_int, n_leng,
norm) so callers/flow remain clear.
| if(i_curve < 9) | ||
| sprintf(buffer, "%s.0%d", iv_file_prefix, i_curve+1); | ||
| else | ||
| sprintf(buffer, "%s0%d", iv_file_prefix, i_curve+1); |
There was a problem hiding this comment.
Inconsistent filename formatting logic.
For i_curve < 9, the format is "%s.0%d" (with a dot), but for i_curve >= 9, the format is "%s0%d" (without a dot). This creates inconsistent filenames: curve 8 → prefix.09, curve 9 → prefix010. The condition and format strings need alignment.
Suggested fix (assuming sequential numbering 01-99+)
/* prepare file prefix */
- if(i_curve < 9)
- sprintf(buffer, "%s.0%d", iv_file_prefix, i_curve+1);
+ if(i_curve + 1 < 10)
+ sprintf(buffer, "%s.0%zu", iv_file_prefix, i_curve+1);
else
- sprintf(buffer, "%s0%d", iv_file_prefix, i_curve+1);
+ sprintf(buffer, "%s.%zu", iv_file_prefix, i_curve+1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(i_curve < 9) | |
| sprintf(buffer, "%s.0%d", iv_file_prefix, i_curve+1); | |
| else | |
| sprintf(buffer, "%s0%d", iv_file_prefix, i_curve+1); | |
| if(i_curve + 1 < 10) | |
| sprintf(buffer, "%s.0%zu", iv_file_prefix, i_curve+1); | |
| else | |
| sprintf(buffer, "%s.%zu", iv_file_prefix, i_curve+1); |
🤖 Prompt for AI Agents
In @src/rfac/rfrmin.c around lines 304 - 307, The filename formatting is
inconsistent between the two branches (uses "%s.0%d" when i_curve < 9 but
"%s0%d" otherwise), producing mismatched names like "prefix.09" vs "prefix010";
replace the two-branch logic with a single consistent format that includes the
dot and zero-pads the curve number (use iv_file_prefix and i_curve+1) — e.g.,
use a single sprintf targeting buffer with a format that always includes the dot
and pads to two digits (reference iv_file_prefix, i_curve, buffer).
| /* Write IV curves with best agreement to files */ | ||
| rfac_ivcur_write(args->iv_file, iv_cur, n_leng, *r_min_ptr, shift, | ||
| *e_range_ptr, e_range, args->r_type, args->vi); |
There was a problem hiding this comment.
Incorrect arguments passed to rfac_ivcur_write.
Multiple argument issues in this call:
shiftis the last value from the loop, not the best shift (*s_min_ptr) that produced the minimum R-factor.*e_range_ptris passed as thedeparameter, butdeshould be the energy step (args->s_step).e_rangeis passed astotal_energy_range, bute_rangeholds only the value from the last loop iteration, not the cumulative range at the best shift.
Looking at rfac_ivcur_write signature: (prefix, iv_cur, n_leng, r_min, shift, de, total_energy_range, r_type, vi)
Suggested fix
/* Write IV curves with best agreement to files */
- rfac_ivcur_write(args->iv_file, iv_cur, n_leng, *r_min_ptr, shift,
- *e_range_ptr, e_range, args->r_type, args->vi);
+ rfac_ivcur_write(args->iv_file, iv_cur, n_leng, *r_min_ptr, *s_min_ptr,
+ args->s_step, *e_range_ptr, args->r_type, args->vi);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* Write IV curves with best agreement to files */ | |
| rfac_ivcur_write(args->iv_file, iv_cur, n_leng, *r_min_ptr, shift, | |
| *e_range_ptr, e_range, args->r_type, args->vi); | |
| /* Write IV curves with best agreement to files */ | |
| rfac_ivcur_write(args->iv_file, iv_cur, n_leng, *r_min_ptr, *s_min_ptr, | |
| args->s_step, *e_range_ptr, args->r_type, args->vi); |
🤖 Prompt for AI Agents
In @src/rfac/rfrmin.c around lines 448 - 450, The call to rfac_ivcur_write
passes the wrong variables: replace the loop-local shift and e_range with the
best-shift and the correct energy values—use *s_min_ptr for the shift argument,
use args->s_step for the de (energy step) argument, and use *e_range_ptr for the
total_energy_range argument while keeping iv_cur, n_leng, *r_min_ptr,
args->r_type, and args->vi as before.
|
1 participant
Summary
This PR unifies the work from:
feat/cleed-core) - Core library with curve helpers and IO stubfeat/issue-48-cleedpp) - C++ bindings libraryChanges included:
cleed_core library (
src/cleed_core/)cleed_iolibrary for future file parsingcleed++ C++ bindings (
src/cleed++/)extern "C"guards added to public C headers for C++ compatibilityDocumentation
doc/cpp_bindings.rst)TODOdirectives to lowercasetodoBuild integration
BUILD_CPP_BINDINGSoption for optional C++ supportTesting
BUILD_CPP_BINDINGS=ONcmake -S . -B build -DBUILD_CPP_BINDINGS=ON cmake --build build --parallel ctest --test-dir build --output-on-failureSupersedes
Follow-ups
Summary by CodeRabbit
New Features
Documentation
Build System
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.