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

Conversion of most C files to C++ #1203

Merged
merged 11 commits into from Dec 26, 2018

Conversation

Projects
None yet
3 participants
@rouault
Copy link
Member

rouault commented Dec 18, 2018

No description provided.

@rouault rouault force-pushed the rouault:cpp_conversion branch 5 times, most recently from 5a7aa38 to 0f41ec3 Dec 18, 2018

@rouault rouault changed the title [WIP] cpp conversion Conversion of all C files to C++ Dec 18, 2018

@rouault

This comment has been minimized.

Copy link
Member

rouault commented Dec 18, 2018

OK, so now all warnings are fixed. Some further tidy'ification could be done to convert from example old-style C casts to C++ ones, but that can be job for later.

@rouault

This comment has been minimized.

Copy link
Member

rouault commented Dec 18, 2018

One thing I didn't do is to run scripts/reformat_cpp.sh ... Do we want to reformat the existing codebase ?

@kbevers
Copy link
Member

kbevers left a comment

I am surprised that it didn't take more to make this change. Apart from one in-lined comnent I have nothing with regards to the actual changes you have made in this PR. I do however have a two additional related points that I think is worth considering:

1. Re-organize files in src:

The current structure of operations being prefixed by PJ_ and other code files prefixed with pj_ (apart from really old files and CLI apps) is not particularly smart in my opinion. Since all files are being renamed we have the chance to do something about this withouth obscurring the git history more than necessary. A quick proposal for reorganization could be a file tree something like this:

src/apps/				# proj, geod, cs2cs, cct, gie, and projinfo
src/conversions/		# all conversion files, unitconvert, axisswap, etc. 
src/projections/		# all projection files, merc, utm, etc.
src/transformations/ 	# all transformations, helmert, molodensky, etc.
src/					# ... and leave the rest of the files where they are

This is just a quick suggestion, I am sure more can be done to improve the structure.

2. Remove PJ_ and pj_ prefixes files:

If the files are better organized the need for prefixes in the filenames is no longer there there. So we might as well remove them. Note that at the moment we have two projections that are not prefixed with PJ_: proj_etmerc.c and proj_rouss.c. In any case these should be taken care of as part of the big renaming in this PR. Similarly for PJ_nocol.c that really should be called PJ_nicol.c.


namespace { // anonymous namespace

This comment has been minimized.

@kbevers

kbevers Dec 19, 2018

Member

Excuse my poor C++ skills, is it necessary for Mode and pj_opaque to live in it's own namespace or is this just a consequence of an automatic conversion?

This comment has been minimized.

@rouault

rouault Dec 19, 2018

Member

It is needed for the reason mentioned in the commit message of 9bbdffd

Defining struct pj_opaque with different definitions is a violation
of the C++ One-Definition-Rule. When using link-time optimizations, this
could break badly.
The solution adopted here is to wrap those structures into a C++
anonymous namespace so they are considered different

This comment has been minimized.

@kbevers

kbevers Dec 19, 2018

Member

Ah okay, I missed that. Thanks for the explanation.

/******************************************************************************
*
* Project: PROJ
* Purpose: Dummy test to check that we can include proj.h from a pure C file

This comment has been minimized.

@kbevers

kbevers Dec 19, 2018

Member

Good idea!

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Dec 19, 2018

One thing I didn't do is to run scripts/reformat_cpp.sh ... Do we want to reformat the existing codebase ?

I guess we should since we have already agreed that C++ code follows the format defined in reformat_cpp.sh. But this is a bit out of the ordinary so it would be good to have an idea of the impact it has on the code. Parts of the code base has been carefully formatted to improve readability. Those parts will very likely be obscurred and left worse than it started. This maybe accounts for 10% of the C code base. The rest is more fluffy and will probably be in a better state after a reformat.

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Dec 19, 2018

@cffk Is there something we need to consider regarding the geodesic library? With this change, will we be better off using your C++ code instead?

@rouault rouault force-pushed the rouault:cpp_conversion branch 3 times, most recently from 6c70791 to 5d3b900 Dec 19, 2018

@cffk

This comment has been minimized.

Copy link
Contributor

cffk commented Dec 20, 2018

@kbevers I recommend leaving the geodesic routines in PROJ as is for
now. The C version is nicely self-contained, and, evidently, the C++
compiler doesn't complain about it.

However... I worry that the conversion of PROJ to C++ will cause
headaches for clients of PROJ. I much prefer programming in C++.
However, C libraries present a much cleaner interface to the outside
world; there's no name mangling and the system runtime library support
is usually automatic.

A big advantage of the current C-based PROJ is that lots of other
packages can use it directly and providing interfaces in other languages
is rather straightforward. Some of these advantages will go away with a
C++ library.

The problem becomes acute on Windows platforms. The current PROJ
library can be used by all(?) versions of Visual Studio and with debug
and release modes. But a C++ library is specific to a particular
version of Visual Studio and you need separate debug and release
versions.

I'm sorry that I haven't been keeping track of the discussion leading up
to this C++ conversion effort. So it's possible that these concerns
have already been addressed.

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Dec 20, 2018

I'm sorry that I haven't been keeping track of the discussion leading up
to this C++ conversion effort. So it's possible that these concerns
have already been addressed.

Don't be sorry! Everything has been moving quite fast for the last six months and it is certainly hard to keep up to date with everything.

Your concerns has mostly been discussed before but that doesn't mean they are not valid. @rouault's ISO19111 work more or less requires an object oriented programming language. At least from a practical point of view (sure you could do the same in C, but not as simple, safe or fast as doing it in C++). We are of course still exposing the API in C header so it should still be straight forward to use the library. We loose the projects that are inclined to incorporate the PROJ codebase in their own that require C code only. The changes in this PR doesn't make any difference in that regard, as that constraint was already broken a few months ago when the first ISO19111 code was merged. We have touched on this in previous discussion and came to the conclusion that this was a loss that we would have to accept. It shouldn't affect too many projects I hope, especially since most package managers tend to link to the distributed version of PROJ and not the project-specific version that may be included in a source distribution of a given software package.

Regarding compilers on Windows, the most recent version of Visual Studio is freely available for open source usage so this shouldn't really be a big issue on that platform. Users that for some reason can't use a modern compiler can probably also live with an older version of PROJ.

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Dec 20, 2018

I recommend leaving the geodesic routines in PROJ as is for
now. The C version is nicely self-contained, and, evidently, the C++
compiler doesn't complain about it.

Right. We shall do that then. Should you not wish to maintain two versions of your code in the future at least you have the option to use the C++ instead.

@rouault rouault force-pushed the rouault:cpp_conversion branch from f1f7ecb to 70790a8 Dec 20, 2018

@rouault

This comment has been minimized.

Copy link
Member

rouault commented Dec 20, 2018

OK, I've reverted geodesic.cpp to geodesic.c (not a total revert, since that incorporates a few improvements from the C++ temporary conversion, like nullptr readyness and some 0U instead of GEOD_NONE usage)

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Dec 25, 2018

I hear no objections to this PR. @rouault do you mind fixing the merge conflicts so we can get this merged?

@rouault rouault force-pushed the rouault:cpp_conversion branch from 70790a8 to d1a6475 Dec 26, 2018

@rouault rouault changed the title Conversion of all C files to C++ Conversion of most C files to C++ Dec 26, 2018

@rouault

This comment has been minimized.

Copy link
Member

rouault commented Dec 26, 2018

Rebased on top of current master

rouault added some commits Dec 18, 2018

cpp conversion: fix One-Definition-Rule violations
Defining struct pj_opaque with different definitions is a violation
of the C++ One-Definition-Rule. When using link-time optimizations, this
could break badly.
The solution adopted here is to wrap those structures into a C++
anonymous namespace so they are considered different

rouault added some commits Dec 19, 2018

cpp conversion: move source files in apps/ iso19111/ conversions/ pro…
…jections/ transformations/ tests/ subdirectories

@rouault rouault force-pushed the rouault:cpp_conversion branch from d1a6475 to 80dad6e Dec 26, 2018

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Dec 26, 2018

Thanks!

@kbevers kbevers merged commit 81ec8c0 into OSGeo:master Dec 26, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Dec 26, 2018

Just for the fun of it, I have run scripts/reformat_cpp.sh on the whole code base. I am evaluating the results now. Feel free to have a look for yourself: https://github.com/OSGeo/proj.4/compare/master...kbevers:cpp-reformat?expand=1

My initial reaction is that it is not too bad but clang-format obscures some larger comment sections. There may be ways to get around that.

@kbevers kbevers added this to the 6.0.0 milestone Dec 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment