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

Modernise C++ code #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Modernise C++ code #37

wants to merge 1 commit into from

Conversation

nilason
Copy link
Collaborator

@nilason nilason commented Jan 30, 2024

This modernises the C++ code, removing as much C style coding as possible.

I took heavy use of Clang-Tidy, Cppcheck, scan-build to implement this.
It passes the new tests of #36.

grass.cpp Outdated Show resolved Hide resolved
grass.cpp Outdated Show resolved Hide resolved
ogrgrasslayer.cpp Outdated Show resolved Hide resolved
@nilason
Copy link
Collaborator Author

nilason commented Feb 4, 2024

@rouault Thanks for looking at this! Do you deem this safe for a patch release, i.e. 1.0.3?

@rouault
Copy link
Member

rouault commented Feb 4, 2024

Do you deem this safe for a patch release, i.e. 1.0.3?

This is your call, but IMHO no.

@nilason
Copy link
Collaborator Author

nilason commented Feb 4, 2024

Do you deem this safe for a patch release, i.e. 1.0.3?

This is your call, but IMHO no.

I agree it is not, I suggested this for 1.1.0 in #38. Very much value your opinion though!

OGRFeature *GetNextFeature() override;
OGRFeature *GetFeature(GIntBig nFeatureId) override;
virtual auto SetNextByIndex(GIntBig nIndex) -> OGRErr override;
auto GetNextFeature() -> OGRFeature * override;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out interest, what guideline you are using for applying the auto...-> return_type syntax generally? (as opposed to just places where it is needed because the type is not known beforehand)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a purely stylistic feature introduced with C++11. In my opinion, coloured after getting accustomed with it in Swift, it provides a bit cleaner interface (lifts up the function/method names in the declaration).
Clang-tidy has a check for this: https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-trailing-return-type.html.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fits with the type annotations in Python which is nice (although auto being in place of def is little strange). I just haven't seen a guideline suggesting to apply it generally. With a automated check available - at least in clang-tidy - I can see this can be realistically applied to the code base.

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.

3 participants