cppcheck issues in vector/* [DUP from discourse]

[gulshan-123]

Moving the discussion from OSGeo discourse to github :) cc @wenzeslaus

---

Most of the cppcheck issues in vector/* are not a problem at runtime

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: see 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. Instead it is suggesting to use !last_row.

[Similar errors (1 & 2) 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;
            ^

It is used in the function

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;
}

Here it is giving warning that the memory may leak if we try to access memory AT return_value. But I've verified that we are careful in its usage. Also, all the memory is freed correctly using the corresponding Pvector_free().
But yes, this function and its usage can be refactored to use the starting pointer of allocated memory instead of some subtracted pointer.

Should we rewrite these portions of code so that warnings are absent, or just we ignore these warnings?

[gulshan-123]

@wenzeslaus , Just a gentle reminder in case this was missed. (Also please tell if we should discuss on discourse)

Thank You!

[wenzeslaus]

Most of the cppcheck issues in vector/* are not a problem at runtime

Checking vector/v.lidar.correction/main.c …
vector/v.lidar.correction/main.c:449:9: warning: Uninitialized variable: npoints [uninitvar]
if (npoints > 0) {
         ^

I think here it is the case which is hard to read for both the machine and a human. Is npoints used or initialized in P_Read_Vector_Correction? Well, one needs to check the function or its doc. Perhaps a completely different code structure would me it obvious, but with the given code structure, I think assigning 0 to initialize indicates that we expect something else to set that to non-zero (as opposed to we possibly forgot to initialize).

Generally, forgetting we only did npoints=0, but never assigned a desired value is a less tricky bug to navigate then uninitialized variable. The difference is between never doing anything versus something not doing anything and sometimes behaving is a random way, perhaps crashing.

This is an important consideration because we may hope for a better checks which would see a more complex initialization (like P_Read_Vector_Correction). In that scenario, having npoints=0 may actually then hide a later bug in the complex initialization. However, even in that case, we are at least left with the "less tricky bug" from the above, so I would say that's acceptable tradeoff: "less tricky bug" now and in the future, while loosing theoretical future warning.

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. Instead it is suggesting to use !last_row.

While there is possible stylistic issue, we don't check for those at this point. This is about what code path it is considering, read: note: Assuming condition is false.

vector/v.lidar.growing/ConvexHull.c:246:5: error: Memory leak: v [memleak]
return v - nl + NR_END;
...use the starting pointer of allocated memory instead of some subtracted pointer.

Returning a subtracted pointer and "forgetting" the actual beginning seems strange and it is hard to read. I would have to look more into the code beyond the function itself to understand the reasons there, but something which would be more explicit would be nicer.

Should we rewrite these portions of code so that warnings are absent, or just we ignore these warnings?

Ideally, rewrite, but at the same time, we don't want to introduce a bug in the algorithm while fixing what is more or less a readability issue, so consider adding tests and/or focus on places with tests.