I am looking at the GSoC webpage of GRASS, and all the ideas are looking great!
Thus, I’m exploring more and more about each of ideas to choose which I can contribute the most effectively.
I am currently familiar with adding JSON and other formats to grass tools.
Currently, I am looking at the cppcheck issue (Topic: Fix known code defects)
I have run cppcheck inside vector directory (cppcheck vector/*) and found some issues:
``
Checking vector/v.lidar.edgedetection/main.c …
vector/v.lidar.edgedetection/main.c:484:9: warning: Uninitialized variable: npoints [uninitvar]
if (npoints > 0) {
^
vector/v.lidar.edgedetection/main.c:335:21: note: Assuming condition is false
while (last_row == FALSE) { /* For each row */
^
vector/v.lidar.edgedetection/main.c:484:9: note: Uninitialized variable: npoints
if (npoints > 0) {
^
``
Checking vector/v.lidar.growing/ConvexHull.c …
vector/v.lidar.growing/ConvexHull.c:246:5: error: Memory leak: v [memleak]
return v - nl + NR_END;
^
```
and some more…
Am I going in correct direction? Are these the issues which we need to solve (apart from other security issues as explained in topic)?
1 Like
Yes, these are the the issues. I suggested Cppcheck for the newcomers because you don’t need to get any special access or set up your own CI to see these. I think we had 0 Cppcheck issues at some point not so long ago, but as the tool gets better and our code more readable, new errors appear (but I’m just guessing here). Some of these may not be real issues if you consider the parameters of the tool, but the enforcement of combination of parameters is not visible to the checkers (too complex, distant and involves string parsing). So even if it is not issue for the standard runtime, but still an issue for maintenance.
For memory leaks, there is more carefulness needed, or rather, a PR may trigger more discussion because previously, the code was written so that the memory at the end of the tool runtime is not freed (because it is freed by the operating system automatically). We changed the approach and already merged PRs which are implementing it at specific places in the code, but we are doing it on case-by-case basis.
(sorry for bad formatting, I’m more comfortable on github formatting)
Yes, most of them are not real issues:
for example:
- Checking vector/v.lidar.correction/main.c …
vector/v.lidar.correction/main.c:449:9: warning: Uninitialized variable: npoints [uninitvar]
if (npoints > 0) {
^
→ Here, npoints is passed as a pointer to the function(before the above line), where it is initialised: L 338
observ = P_Read_Vector_Correction(&In, &elaboration_reg, &npoints,
&nterrain, dim_vect, &lcat);
- vector/v.lidar.correction/main.c:308:21: note: Assuming condition is false
while (last_row == FALSE) { /* For each row */
^
→ This, I believe is just some style warning, as just before this while condition last_row is assigned FALSE, (as its value may change to TRUE within the loop)
[Similar errors in vector/v.lidar.edgedetection/main.c]
-
vector/v.lidar.growing/ConvexHull.c:246:5: error: Memory leak: v [memleak]
return v - nl + NR_END;
^
→ This warning is also not correct, as I have checked that in all the uses of the function where this is written (function name: Pvector), in all the exit paths, the memory is freed correctly using corresponding Pvector_free().
``double **Pvector(long nl, long nh)
{
double **v;
v = (double **)calloc((size_t)(nh - nl + 1 + NR_END), sizeof(double *));
if (!v)
nrerror(“allocation failure in dvector()”);
return v - nl + NR_END;
}
```
This warning IMO just is to warn us using accessing return value memory, as memory is allocated starting from v (not from v minus something). But we have taken care of that in our code.
Should we add these false warnings anywhere or update the code without having these warnings?