Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tools/cppcheck.sh and make it a AZP job #328

Merged
merged 5 commits into from Sep 7, 2020

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Jun 27, 2020

And fixes related warnings

Adapted from script used by GDAL, PROJ, QGIS

@rouault
Copy link
Contributor Author

rouault commented Sep 3, 2020

Any interest from the core team for that contribution, before I rebase it to latest master ?

@pramsey
Copy link
Member

pramsey commented Sep 3, 2020

Seems OK to me? Wonder if we can thin out the AZP matrix so it runs in less that fifty billion hours...?

@rouault
Copy link
Contributor Author

rouault commented Sep 4, 2020

Wonder if we can thin out the AZP matrix so it runs in less that fifty billion hours...?

a bit of a different topic, but for the Linux builds, you could potentially just test the lower and higher gcc & clang versions, and hope that in-between ones are OK...

@rouault
Copy link
Contributor Author

rouault commented Sep 4, 2020

Rebased on top of latest master

@rouault
Copy link
Contributor Author

rouault commented Sep 5, 2020

Note: the CI failures are unrelated to my changes. The same ones occur with master

@Algunenano
Copy link
Contributor

Any interest from the core team for that contribution, before I rebase it to latest master ?

Not a core contributor, but I think having this kind of tooling integrated in the CI it's great, in many situations it forces the code to be simpler (and more modern / C++11-esque) and can detect bugs that otherwise are hard to see.

The one thing I'm not convinced about, but it's totally personal preference, is the postfixOperator. I prefer i++ vs ++i, but I'm happy either way if something detects it for me (although ideally it'd be great to have this integrated as part of a the linter / clang format).

@rouault
Copy link
Contributor Author

rouault commented Sep 7, 2020

is the postfixOperator

the rationale given for this one is that the post-fix operator involves more work in its implementation than the pre-fix one. For the post-fix one, you need to create a copy of an object (for incrementation not on base types). So there's a tiny performance gain in using the pre-fix one.

@pramsey
Copy link
Member

pramsey commented Sep 7, 2020

I'm going to take it in, let the CI zoo continue to grow :)

@pramsey pramsey merged commit 7f83754 into libgeos:master Sep 7, 2020
@rouault
Copy link
Contributor Author

rouault commented Oct 1, 2020

@pramsey I'm confused: this pull request is supposed to have been merged, but I can't see any trace of it in master (here on github I mean). Can't find cppcheck.sh in https://github.com/libgeos/geos/tree/master/tools ...

@pramsey
Copy link
Member

pramsey commented Oct 1, 2020

I think I mashed a merge button maybe, instead of doing the manual two-step to push to the osgeo repo. Sorry about that, it's done now.

@rouault
Copy link
Contributor Author

rouault commented Oct 2, 2020

Sorry about that

thanks! (To be clear my rant wasn't about an occasional mess up, but against the process that makes it possible)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants