Some issues found by scan-build #238

Algunenano · 2019-10-08T18:08:02Z

Used: scan-build make

If you are interested I can try to address the rest of the issues.

Remove use after move semantics (in the asserts).

GEOSLength_r might not set length, and it's used in the next call

Algunenano · 2019-10-08T18:14:44Z

Pending warnings:

dbaston · 2019-10-08T18:14:08Z

capi/geos_ts_c.cpp

@@ -6266,7 +6266,7 @@ extern "C" {
                            const Geometry* p)
    {

-        double length;
+        double length = NAN;


We recently got rid of the NAN macros, so if NAN is useful here maybe use std::numeric_limits<double>::quiet_NaN() or whatever it is? I don't think the default value is actually important because I don't think GEOSLength is capable of producing an error.

This isn't using getLength(), it's using GEOSLength_r which can produce errors if the handle or the geometry are NULL.

I've used NAN here because length is then used as a denominator in a function. The other option would be to check the output of GEOSLength_r and return an error somehow in GEOSProjectNormalized_r, GEOSInterpolateNormalized_r; but I don't know what would a proper value be in that case (0?, NAN?).

Yes, better to check the return of GEOSLength_r and propagate the error value of GEOSProject_r / GEOSInterpolateNormalized_r.

Looking at the code it seems that the sane thing to do would be to return -1 in GEOSProjectNormalized_r (the return error for GEOSProject_r) and 0 in GEOSInterpolateNormalized_r (the return error for GEOSInterpolate_r), right?

Yes, that's what I was trying to say.

src/io/WKTReader.cpp

scan-build warning: DistanceOp.cpp:277:28: warning: Dereference of null smart pointer of type 'std::unique_ptr' const Coordinate& pt = ptLoc->getCoordinate(); Since the content of loc was moved but still used in the loop, it might happen `!(minDistance <= terminateDistance))` that loc was used in the next iteration where its contents were nullified by the previous move. By returning in the same function we let the compiler understand that we are already done

Apparently is unable to see that nPts can't be 0

Algunenano · 2019-10-15T17:31:27Z

There is still 3 warnings left. The idea is to first clean these warnings and then add a new CI target that runs scan-build so it's run automatically.

dbaston · 2019-10-15T17:53:11Z

src/algorithm/MinimumBoundingCircle.cpp

@@ -299,23 +299,23 @@ MinimumBoundingCircle::pointWitMinAngleWithX(std::vector<Coordinate>& pts, Coord
 Coordinate


Was the dereference even necessary in the first place, or should the function just return const Coordinate* ?

You are right, in fact now that I'm looking at it there is a way less intrusive way to handle this which is to just initialize minAngPt to a pointer to the first element, instead of nullptr.

But why does this function need to return a copy?

You are absolutely right. All the other functions do the same, should I change the 3 of them?

Now that I look into it, this would involve major changes in the API, changing extremalPts to an array of references / pointers and so on, so I'd rather do it in a different PR and leave this one with the copy for now.

Sounds good. While you're in there, the other function arguments should probably be const Coordinate & instead of mutable...

scan-build points out that, under some circunstances, it'd possible for minAngPt no never be set (thus being nullptr) and that would mean dereferencing a NULL ptr when returning the object.

dbaston · 2019-10-30T13:55:12Z

@Algunenano are you still working here, or should this branch be merged (I'll squash out the back-and-forth commits first, unless you want to)

Algunenano · 2019-10-30T14:06:54Z

@Algunenano are you still working here, or should this branch be merged (I'll squash out the back-and-forth commits first, unless you want to)

I'm still working on it when I can. There are still a few (2-3?, I don't recall exactly) issues I want to tackle and then I want to try and add the check to the CI.

CoordinateOperation.cpp:50:12: warning: Called C++ object pointer is null return geometry->clone(); ^~~~~~~~~~~~~~~~~

ShortCircuitedGeometryVisitor.cpp:40:13: warning: Forming reference to null pointer visit(*element); ^~~~~~~~~~~~~~~

Algunenano · 2019-11-08T15:47:35Z

I'm currently working on 2 missing things:

Some warnings in ttmath, which I'm not sure how to fix it or ignore it. I've tried updating the library to its current trunk but the warnings are still there.
Create a build in Travis running with scan-build. Right now I'm fighting against C++11 flags, which for some reasons it's not getting automatically and the build fails.

dbaston · 2019-11-08T16:06:50Z

Some warnings in ttmath, which I'm not sure how to fix it or ignore it. I've tried updating the library to its current trunk but the warnings are still there.

Yes, I also spent some time looking at how to conditionally disable some warnings in this header. It seemed like there should be a way to do it by getting CMake to treat it as a "system" header, but I didn't get it to work. For scan-build, does it make sense to just create a suppressions file?

Create a build in Travis running with scan-build. Right now I'm fighting against C++11 flags, which for some reasons it's not getting automatically and the build fails.

I think the longer-term vision is to use AzP, so you might try making the changes there instead.

Algunenano · 2019-11-08T16:19:37Z

Yes, I also spent some time looking at how to conditionally disable some warnings in this header. It seemed like there should be a way to do it by getting CMake to treat it as a "system" header, but I didn't get it to work. For scan-build, does it make sense to just create a suppressions file?

I'll give a try to #pragma GCC system_header and see how it goes, that should get rid of all warnings related to that file and might help scan-build too.

I think the longer-term vision is to use AzP, so you might try making the changes there instead.

For now I'm using Travis because I already know it and had it activated in my fork, but I'll consider switching once I get everything working.

Algunenano · 2019-11-08T16:35:12Z

scan-build appears to ignore my demands of using system_header, but t seems assigning default constructors to ttmath::Big members is enough to please it, so I'll go with that route instead.

Algunenano · 2019-11-08T16:48:07Z

Ok, with this I get a clean build now and there should be a Travis task (https://travis-ci.com/libgeos/geos/jobs/254533462) that only passes if scan-build doesn't throw any warning.

Algunenano · 2019-11-28T14:20:11Z

@dbaston I'd say this is ready for final review now. Let me know if I should improve anything to get it merged.

Algunenano added 5 commits October 8, 2019 19:50

SimpleWKTTester: Memory leaks

9cc2946

UnaryUnionOp: Improve move semantics

8b58427

Remove use after move semantics (in the asserts).

LineSegment: Remove dead assigment

8fff707

nearestEndpoint: Remove dead assigment

793631c

geos_ts_c: Avoid working with garbage

8d21be9

GEOSLength_r might not set length, and it's used in the next call

Algunenano changed the title ~~Some issues find by scan-build~~ Some issues found by scan-build Oct 8, 2019

WKTReader::getCoordinates: Remove reduntant move

36c27c3

dbaston reviewed Oct 8, 2019

View reviewed changes

geos_ts_c: Handle GEOSLength_r errors

3515508

Algunenano force-pushed the scanbuild branch from 272745f to 3515508 Compare October 9, 2019 15:20

Algunenano added 3 commits October 11, 2019 17:23

WKTReader::getCoordinates: Play nice with old compilers

a39f5bd

Orientation: Give scan-build some help

87b7031

Apparently is unable to see that nPts can't be 0

dbaston reviewed Oct 15, 2019

View reviewed changes

Avoid null dereference in pointWithMinAngleWithSegment

64de7ca

scan-build points out that, under some circunstances, it'd possible for minAngPt no never be set (thus being nullptr) and that would mean dereferencing a NULL ptr when returning the object.

Algunenano force-pushed the scanbuild branch from 887299c to 64de7ca Compare October 16, 2019 09:40

dbaston added the WIP Work in progress, do not merge. label Oct 30, 2019

Algunenano added 2 commits November 8, 2019 15:29

CoordinateOperation: Silence scan-build warning

97cdccf

CoordinateOperation.cpp:50:12: warning: Called C++ object pointer is null return geometry->clone(); ^~~~~~~~~~~~~~~~~

ShortCircuitedGeometryVisitor: scan-build warning

2a47460

ShortCircuitedGeometryVisitor.cpp:40:13: warning: Forming reference to null pointer visit(*element); ^~~~~~~~~~~~~~~

Algunenano added 2 commits November 8, 2019 17:37

Scanbuild: Give ttmathbig member default values

1ef0d44

Add a scan-build run to travis

762812e

Fix PR indentation

a12ffb1

strk merged commit a12ffb1 into libgeos:master Dec 2, 2019

nilason mentioned this pull request Dec 2, 2019

DoubleBits, avoid garbage value #252

Closed

pramsey mentioned this pull request Nov 14, 2022

3.11.1: test suite is failing in test_docs unit #734

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some issues found by scan-build #238

Some issues found by scan-build #238

Algunenano commented Oct 8, 2019

Algunenano commented Oct 8, 2019

dbaston Oct 8, 2019 •

edited

Loading

Algunenano Oct 9, 2019

dbaston Oct 9, 2019

Algunenano Oct 9, 2019

dbaston Oct 9, 2019

Algunenano commented Oct 15, 2019

dbaston Oct 15, 2019

Algunenano Oct 16, 2019

dbaston Oct 16, 2019

Algunenano Oct 17, 2019

Algunenano Oct 17, 2019

dbaston Oct 17, 2019

dbaston commented Oct 30, 2019

Algunenano commented Oct 30, 2019

Algunenano commented Nov 8, 2019

dbaston commented Nov 8, 2019

Algunenano commented Nov 8, 2019

Algunenano commented Nov 8, 2019

Algunenano commented Nov 8, 2019

Algunenano commented Nov 28, 2019

		@@ -299,23 +299,23 @@ MinimumBoundingCircle::pointWitMinAngleWithX(std::vector<Coordinate>& pts, Coord
		Coordinate

Some issues found by scan-build #238

Some issues found by scan-build #238

Conversation

Algunenano commented Oct 8, 2019

Algunenano commented Oct 8, 2019

dbaston Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Algunenano commented Oct 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaston commented Oct 30, 2019

Algunenano commented Oct 30, 2019

Algunenano commented Nov 8, 2019

dbaston commented Nov 8, 2019

Algunenano commented Nov 8, 2019

Algunenano commented Nov 8, 2019

Algunenano commented Nov 8, 2019

Algunenano commented Nov 28, 2019

dbaston Oct 8, 2019 •

edited

Loading