Fix -Wshadow warning in src/algorithm/InteriorPointArea.cpp

[rouault]

Submitted as ​https://github.com/libgeos/geos/pull/147 and ​https://git.osgeo.org/gitea/geos/geos/pulls/70

[mdavis]

Which build path does this show up in?

[rouault]

Replying to mdavis:

Which build path does this show up in?

Sorry, I don't understand your question

[mdavis]

I don't see the warning because I'm using clang (OSX). So wondering how to check for these warnings. Does it show up on one of the buildbots?

This is going to be painful going forward unless we can get this check happening in all buildchains.

[rouault]

Ah, I saw this locally with gcc 5.4 of Ubuntu 16.04, but it also showed up on Travis-CI. For example: ​https://travis-ci.com/libgeos/geos/jobs/179892298

We should probably harden bots to enable -Werror

[mdavis]

Yes, no point in issuing warnings which won't be noticed.

But that won't fix my buildchain. So probably need to add those flags to clang as well?

[rouault]

Unfortunately (or fortunately !) gcc has warnings that clang doesn't have, and the reverse. Or sometimes they have the same flags, like -Wshadow, but they don't behave exactly the same, and that also depends on the version of the compilers... That's the point of having CI testing both...

[mdavis]

In that case I think we should remove the warnings which can't be reproduced in all development environments. Otherwise we're going to keep running into this kind of situation. And I don't think you want to keep having to create patches for my code.. :)

[dbaston]

Doesn't Even's suggestion to enable -Werror in our CI solve the reproducibility problem?

[mdavis]

Not sure. If the OSX clang build doesn't have -Wshadow then would -Werror actually raise an error?

[dbaston]

You wouldn't get an error locally, but you'd see it when Travis builds against gcc (​https://travis-ci.com/libgeos/geos/jobs/179892298#L708) and could address it before merging.

[mdavis]

That's a workflow hassle, to have to make a PR and then iterate on that to get the code right.

[rouault]

Replying to mdavis:

That's a workflow hassle, to have to make a PR and then iterate on that to get the code right.

That's the whole point of the github/gitlab/etc + continuous integration workflows... A lot of project work like that. For example, I had to fix the Windows build because there was an earlier commit made in master which broke it.

[strk]

Are we guaranteed that shadowing behaves in the same exact way between Java and any implementation of C++ ?

[rouault]

Are we guaranteed that shadowing behaves in the same exact way between Java and any implementation of C++ ?

Of course not. As mentionned above, even C++ compilers do not all agree on what is shadowing and what is not. You fix until they are all happy...

[strk]

I agree with Martin about the hassle, btw :)

But CI is still good, as long as it cannot represent a blocker (bots can be wrong)

[rouault]

as long as it cannot represent a blocker (bots can be wrong)

I suppose you don't merge pull requests for which a bot is red, do you ? I don't on the projects I'm responsible for...

[mdavis]

I would say policy is not to merge if a bot is red. If the bot is wrong then it needs to be fixed.

Although if a bot is badly broken (like Bessies are now) then might have to ignore them and carry on.

[strk]

I would say the policy is to look after bots and see if they cought a bug introduced by something you commit.

In other words:

1) commit 2) check if bots are happy

This is different from:

1) wait on bots semaphores 2) merge if they are happy

The reason why I like the first better is that we are still in control. Machines can be sooo rude.

In recent projects I work on Travis drives me nuts, it is often unstable for any number of reasons and being unable to merge is wildly frustrating.

[dbaston]

You can run CI on your code in a branch before committing it to master and still be in control. If you decide the failure is because of a problem with the CI, and you aren't able to fix the CI, you can decide to merge it anyway.

"Commit first, fix later" can introduce a lot of frustration when multiple developers are trying to work at the same time. GEOS is not an especially high-traffic project, but there have certainly been times I've stopped work because the master branch had build failures.

[mdavis]

I agree with Dan - best to check bots before committing.

AFAIK the only way to do this in GEOS is via a PR on GitHub - is that right? But that doesn't cover all build platforms, so is not foolproof.

[dbaston]

A GitHub PR gets you testing against Linux (10 build chains) and Windows (2 build chains). I don't think the FreeBSD bots pick up pull requests. And we don't have any testing for OS X (though Travis supports it).

[mdavis]

Ok, that sounds good enough to catch most coding errors. And now that the badges are fixed the GH CI will be a bit more visible.

So I guess the best practice is to always make a GH PR for any non-trivial change. Would be good to document this.

This still only really helps if Werror is in place, right?

[dbaston]

Yes, we still need -Werror unless we want to read all of the build logs (I certainly don't!)

[strk]

The only way to test _TRAVIS_ is via PRs on GitHub. But you can test other bots in other ways.

Drone can be tested locally with:

drone exec

(requires installing drone)

A PR on the OSGeo GIT infrastructure will automatically trigger a Drone build and report status in the commit tab (green/yellow/red tiny dot).

Debbie and Winnie builds I think are only triggered by commits to the "master" branch.

Generally speaking, I think developers are humans so don't need bots and should just run:

make check

which should run all tests in case of GEOS

NOTE that every environment may have its own peculiarities and thus fail in different way. This means a bot can be happy while a Real Human Developer may be blocked by a build failure in his/her specific environment (our bots cover only a subset of operating systems/architectures).

[strk]

I would not like to be _recommended_ to use a proprietary service, so please don't force me, as a developer, to file pull requests on GitHub.

I'm happy with pushing PRs against the official Git hosting service (OSGeo infrastructure).

The Jenkins service run by Regina if I'm not mistaken does support building code from Gitea pull requests, only nobody found the time yet to set it up. Does anyone want to help Regina with that ?

It would also give us FreeBSD testing.

[mdavis]

I agree with strk that the ideal is that a dev just has to check their local build to see if all is good. But that's what I've been doing and it's not been a great experience so far.

One issue is that the autotools build is different to the cmake build (e.g. check for trailing spaces) Another is the one on this ticket - -Wshadow is not in place for OSX build.

So I have to check the bots as well.

(The REAL ideal would be one build chain and one repo - but that's been discussed elsewhere.)

[mdavis]

FWIW, I experimented with add AppleClang to the cmake GCC path (which is pretty crazy in itself -*- ). Had to remove -fno-implicit-inline-templates. Then still got a ton of new warnings, mostly about implicit conversions.

So it doesn't seem like there's a simple path to making the OSX build chain match the GCC one. Which is not to say it shouldn't be done.

(*) ​https://stackoverflow.com/questions/10046114/in-cmake-how-can-i-test-if-the-compiler-is-clang

[rouault]

anything preventing this PR from being merged ? The discussion about -Werror and CI policy are a bit out-of-topic

[Even Rouault <even.rouault@…>]

In 2c426ed/git:

src/algorithm/InteriorPointArea.cpp: fix -Wshadow warning (fixes #954)