Suppress Nurbs warnings #3345

SergioRAgostinho · 2019-09-15T15:33:11Z

While preparing to work on the release items, I noticed nurbs was spewing out an impressive number of warnings. I decided to finally clean it up. There's only a deprecation warning missing.

Situations in which I commented variables instead of simply deleting them are usually a sign that there is commented code in the function's body which originally intended to use them.

Most if not all, ctor base class initializations that I added to suppress the compiler warnings are actually no-ops. Probably the reason they were not being invoked in the first place.

taketwo

This was a huge one, thanks for the effort.

surface/src/3rdparty/opennurbs/opennurbs_bezier.cpp

SergioRAgostinho · 2019-09-16T20:12:21Z

Sorry for the early force push. I forgot about it.

taketwo · 2019-09-17T06:09:46Z

There are a few more sign-compare warnings left in Nurbs, see lines 3716-3806 in Ubuntu CI log, would you mind adding a commit for them as well?

SergioRAgostinho · 2019-09-17T18:43:18Z

Small heads-up. I found it strange that I was not seeing those sign-compare warnings during my build. I'm using g++ 7.4 on a Ubuntu distro. ~~I double checked my CMAKE_CXX_FLAGS and they're empty~~ (only on the ncurses ccmake window, probably because the variable is no longer a CACHE variable). I'm gonna dig around this new issue before proceeding with the last commit on this PR.

SergioRAgostinho · 2019-09-17T20:22:18Z

The ci job reports using gcc 5.4.

-- The C compiler identification is GNU 5.4.0
-- The CXX compiler identification is GNU 5.4.0

I manually compiled gcc 5.5.0 (could not get 5.4.0 to compile) and gave it a try at compiling pcl_surfaceand it's not outputting the sign-compare warnings.

Is the build outputting anything in your environment?

taketwo · 2019-09-17T20:42:20Z

I have GCC 5.5 on my Ubuntu 16 machine and it also does not output sign warnings.

I'm puzzled why did CI job report GCC 5.4 though. It's running on a Ubuntu 16 based container which I re-created just a few days ago, so it should also have GCC 5.5. ~~I will investigate tomorrow.~~ Indeed, the container has 5.4. Also starting from a fresh ubuntu:16.04 container and installing gcc gives me version 5.4.

By the way, here is how you can quickly reproduce a CI run using docker. First, get the image:

docker pull pointcloudlibrary/env:16.04

Second, run the image mapping PCL source directory into it. Assuming you are in the repository root:

docker run --rm -v $PWD:/code -it pointcloudlibrary/env:16.04 /bin/bash

This will get you into a Docker container. Create a build directory:

mkdir /tmp/build && cd /tmp/build

Configure and make:

cmake /code && make

taketwo · 2019-09-27T10:46:11Z

I implemented the changes needed to suppress -Wsign-compare and took the liberty to commit them to your branch. Ubuntu 16.04 job on CI runs clean from NURBS warnings, so I think this is ready to be merged.

kunaltyagi · 2019-09-27T12:16:05Z

Also starting from a fresh ubuntu:16.04 container and installing gcc gives me version 5.4

It might be because the image is cached and you need to pull the base image too. Use --pull in your docker build command to make docker check for a newer base image. (I'm sorry, I haven't built the docker image myself to confirm my hunch)

taketwo · 2019-09-27T12:27:06Z

I tried to docker build --pull the Dockerfile that we have in the repository, but the compiler is still the same, GCC 5.4.0.

Edit: though I can confirm that this pulled a new Ubuntu 16.04 image (that was created just eight days ago).

kunaltyagi

If any of these should be a new PR, please create a new issue (most look like good-to-start).

Otherwise, LGTM. A big changset though, wonderfully organized 😃

surface/src/3rdparty/opennurbs/opennurbs_annotation2.cpp

surface/src/3rdparty/opennurbs/opennurbs_archive.cpp

kunaltyagi · 2019-09-27T13:25:10Z

surface/src/on_nurbs/fitting_curve_2d_apdm.cpp

@@ -1046,7 +1046,7 @@ FittingCurve2dAPDM::findClosestElementMidPoint (const ON_NurbsCurve &nurbs, cons
    double &xi1 = elements[i + 1];
    double dxi = xi1 - xi0;

-    for (unsigned j = 0; j < nurbs.Order (); j++)
+    for (unsigned j = 0; j < static_cast<unsigned int>(nurbs.Order ()); j++)


The casts are all over the place. (to unsigned int, size_t). Maybe only one is required and not all (for nurbs.Order()) variants

surface/src/on_nurbs/fitting_curve_2d_asdm.cpp

SergioRAgostinho · 2019-10-02T15:29:36Z

Sorry for the absence. In situations like this feel free both of you to push stuff to my PR branches whenever required.

SergioRAgostinho · 2019-10-21T18:33:39Z

Oops. Forgot about addressing your comments. The changes LGTM.

I would personally not deprecate or make any other changes considered to be stricly required to 3rd party code but there's indeed the "risk" someone is using directly the OpenNurbs version we're packaging.

… class' warnings

… warning.

…egers warnings

SergioRAgostinho · 2019-10-22T06:59:35Z

I just rebased this branch on top the current master and there still a number of size_t occurrences present in the code. 😕

taketwo

The compilation output on CI is clean, so I guess this one is ready. Thanks for all the effort!

SergioRAgostinho added the module: surface label Sep 15, 2019

taketwo reviewed Sep 15, 2019

View reviewed changes

surface/src/3rdparty/opennurbs/opennurbs_bezier.cpp Outdated Show resolved Hide resolved

SergioRAgostinho force-pushed the nurbs-warnings branch from 3594a66 to 6ee5332 Compare September 16, 2019 20:09

taketwo added the needs: more work Specify why not closed/merged yet label Sep 24, 2019

kunaltyagi mentioned this pull request Sep 27, 2019

Treat compiler warnings as errors #3379

Closed

22 tasks

taketwo removed the needs: more work Specify why not closed/merged yet label Sep 27, 2019

taketwo requested a review from kunaltyagi September 27, 2019 10:46

kunaltyagi reviewed Sep 27, 2019

View reviewed changes

SergioRAgostinho mentioned this pull request Oct 19, 2019

1.10.0 Release #2772

Closed

11 tasks

SergioRAgostinho and others added 11 commits October 22, 2019 08:47

[surface] nurbs - suppress ignored qualifier warnings

5ba196b

[surface] nurbs - suppress unused parameter warnings

0b52127

[surface] nurbs - supress variable set but not used warnings

e1b40da

[surface] nurbs - Suppress 'Copy ctor must explicitly initialize base…

1616c8f

… class' warnings

[surface] nurbs - suppress misleading indentation warnings

848a3d7

[surface] nurbs - suppress fall through warnings

5821d0e

[surface] nurbs - Suppress polymorphic class requiring a virtual dtor…

b4bad4c

… warning.

[surface] nurbs - suppress output from fread not being parsed warning

da44591

[surface] nurbs - suppress enumeral comparison warning

e2c21d6

[surface] nurbs - suppress comparison between signed and unsigned int…

67f2726

…egers warnings

Remove needless comment

4524268

kunaltyagi added 2 commits October 22, 2019 08:50

Add deprecation warning alongside runtime errors

cade0bd

Cast nurb.Order uniformly to std::size_t

8da617e

SergioRAgostinho force-pushed the nurbs-warnings branch from 3872c98 to 8da617e Compare October 22, 2019 06:50

SergioRAgostinho force-pushed the nurbs-warnings branch 2 times, most recently from 78694f4 to a26f471 Compare October 22, 2019 07:55

Add missing size_t -> std::size_t replacements.

a73eae7

SergioRAgostinho force-pushed the nurbs-warnings branch from a26f471 to a73eae7 Compare October 22, 2019 07:59

taketwo approved these changes Oct 29, 2019

View reviewed changes

kunaltyagi merged commit 8d5fa69 into PointCloudLibrary:master Oct 30, 2019

SergioRAgostinho deleted the nurbs-warnings branch November 13, 2019 07:21

kunaltyagi mentioned this pull request Apr 23, 2020

Reduce warnings during compilation with all (usual) warnings switched on #2746

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress Nurbs warnings #3345

Suppress Nurbs warnings #3345

SergioRAgostinho commented Sep 15, 2019 •

edited

Loading

taketwo left a comment

SergioRAgostinho commented Sep 16, 2019

taketwo commented Sep 17, 2019

SergioRAgostinho commented Sep 17, 2019 •

edited

Loading

SergioRAgostinho commented Sep 17, 2019

taketwo commented Sep 17, 2019 •

edited

Loading

taketwo commented Sep 27, 2019

kunaltyagi commented Sep 27, 2019

taketwo commented Sep 27, 2019 •

edited

Loading

kunaltyagi left a comment

kunaltyagi Sep 27, 2019

SergioRAgostinho commented Oct 2, 2019

SergioRAgostinho commented Oct 21, 2019

SergioRAgostinho commented Oct 22, 2019

taketwo left a comment

Suppress Nurbs warnings #3345

Suppress Nurbs warnings #3345

Conversation

SergioRAgostinho commented Sep 15, 2019 • edited Loading

taketwo left a comment

Choose a reason for hiding this comment

SergioRAgostinho commented Sep 16, 2019

taketwo commented Sep 17, 2019

SergioRAgostinho commented Sep 17, 2019 • edited Loading

SergioRAgostinho commented Sep 17, 2019

taketwo commented Sep 17, 2019 • edited Loading

taketwo commented Sep 27, 2019

kunaltyagi commented Sep 27, 2019

taketwo commented Sep 27, 2019 • edited Loading

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi Sep 27, 2019

Choose a reason for hiding this comment

SergioRAgostinho commented Oct 2, 2019

SergioRAgostinho commented Oct 21, 2019

SergioRAgostinho commented Oct 22, 2019

taketwo left a comment

Choose a reason for hiding this comment

SergioRAgostinho commented Sep 15, 2019 •

edited

Loading

SergioRAgostinho commented Sep 17, 2019 •

edited

Loading

taketwo commented Sep 17, 2019 •

edited

Loading

taketwo commented Sep 27, 2019 •

edited

Loading