Modernizing code base: C99 and maybe C++ ?

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Modernizing code base: C99 and maybe C++ ?

Even Rouault-2
Hi,

the kind of topics that is always controversial, but I do think this is a
matter of survival for the project. I can write C89 code for sure, but which
bad thing did I do to still be obliged to do it :-) ? Should new contributors
be constrained by that... 2020 is just in a few days :-)

Do we have a strong reason to stay with C89 for our .c files ? Not being able
to use for loop initial declarations ( that is "for( int i = 0; i < ... ; ++)"
) is the example of something really annoying. Or being able to put
declarations in the middle of a function instead of putting them at the
beginning of it. This makes code harder to read, maintain and more error prone
because of potential accidental reuse of variables that shouldn't.

All gcc and clang compilers from the last 10 years support C99. Regarding
MSVC, MSVC >= 2015 has decent enough support of it. Do we need to support
older MSVC versions ? I don't think so. Neither current GDAL nor PROJ can
built with older versions

For the record, PROJ requires C99 for its few remaining .c files (and C++11
for .cpp. So does GDAL)

Next topic would be a

for i in *.c; do mv $i "`basename $i .c`.cpp"; done

Well, maybe not like that, because I'm expecting a ton of warnings. But more
on a case by case basis where someone needs to do non-trivial changes in part
of the code.

Being constrained by C deeply sucks honesty. Apart from the Linux kernel which
still resists for good reasons, I can't think of a single one for MapServer.

The main point where C sucks is string manipulation, and MapServer does a ton
of it. Being able to use std::string would be such a relief.

Ah for the record, we do have C++ in the code base: mapogr.cpp and the agg
renderer.

Even

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Dropping support for non-GDAL and non-PROJ builds [was Re: Modernizing code base: C99 and maybe C++ ?]

Even Rouault-2
And while I'm a rant-style RFC mode, what about dropping those #ifdef USE_GDAL
and #ifdef USE_PROJ that polutes the code base ? And we don't actually CI test
those configurations, hence my mantra "untested code is broken code"

Who relies on non-PROJ and non-GDAL enabled code ? And if they are a few that
do, do we care about them ? We don't need to be nice with everyone. That
doesn't even buy us free beers.

(I love those rant-style RFCs. They are so pleasant to write :-) And quite to
the point I think !)

Even

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds [was Re: Modernizing code base: C99 and maybe C++ ?]

Seth G-2
Hi Even,

I ran into this the other day when I had the failing FastCGI issue and wanted to debug without Proj to see if that was the cause.
MapServer currently fails to compile unless both -DWITH_PROJ=1 and -DWITH_GDAL=1 are set.

Turning off GDAL also means the following have to be turned off:

-DWITH_CLIENT_WMS=0 -DWITH_WCS=0

I then get the following compile error:

msGetStringListFromHashTable - identifier not found (possibly related to the new layer->connectionoptions)

Turning off PROJ means the following options also have to be turned off:

-DWITH_CLIENT_WMS=0 -DWITH_CLIENT_WFS=0 -DWITH_WFS=0 -DWITH_SOS=0 -DWITH_WMS=0

I then get the following compile error:

'msApproxTransformer': undeclared identifier

Fixing these may of course not resolve all issues.

I agree we should either require PROJ and GDAL, or fix the current errors and add a new CI build without these options to ensure they don't become broken again.

I like the idea of being able to compile a "lightweight" map rendering engine, but on the other hand I've never required one..
It might be worth having a poll on the users list?

Of course by default now both GDAL and PROJ are required so support has effictively been dropped on master at least. If users/companies require them then they'll need to supply a pull request or funding. At present it looks like only a couple of fixes would be needed to allow these builds.

Hope all is well - I'm not sure I've seen a rant by you before!

Seth

--
web:http://geographika.co.uk
twitter: @geographika

On Wed, Nov 27, 2019, at 1:26 AM, Even Rouault wrote:

> And while I'm a rant-style RFC mode, what about dropping those #ifdef USE_GDAL
> and #ifdef USE_PROJ that polutes the code base ? And we don't actually CI test
> those configurations, hence my mantra "untested code is broken code"
>
> Who relies on non-PROJ and non-GDAL enabled code ? And if they are a few that
> do, do we care about them ? We don't need to be nice with everyone. That
> doesn't even buy us free beers.
>
> (I love those rant-style RFCs. They are so pleasant to write :-) And quite to
> the point I think !)
>
> Even
>
> --
> Spatialys - Geospatial professional services
> http://www.spatialys.com
> _______________________________________________
> mapserver-dev mailing list
> [hidden email]
> https://lists.osgeo.org/mailman/listinfo/mapserver-dev
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds [was Re: Modernizing code base: C99 and maybe C++ ?]

Daniel Morissette
On 2019-11-27 08:38, Seth G wrote:
>
> Hope all is well - I'm not sure I've seen a rant by you before!
>
> Seth
>

Ha! Ha! :-)  I've been wondering the same. I checked my calendar and it
was not April 1st yet, and then I actually wondered if it could have
been someone spoofing Even's email address.

Daniel
--
Daniel Morissette
Mapgears Inc
T: +1 418-696-5056 #201
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds [was Re: Modernizing code base: C99 and maybe C++ ?]

Michael Smith
I agree, I had to do a double take myself. But I think it also points out the very real fact that if something like causes Even to rant, that this also has probably contributed to others not wanting to participate or contribute. And may also help confirm the notion for some that MapServer is old code and settled and not still a very active and worthwhile project.

I think this is something we should seriously consider.

Michael Smith
US Army Corps

> On Nov 27, 2019, at 8:51 AM, Daniel Morissette <[hidden email]> wrote:
>
> On 2019-11-27 08:38, Seth G wrote:
>> Hope all is well - I'm not sure I've seen a rant by you before!
>> Seth
>
> Ha! Ha! :-)  I've been wondering the same. I checked my calendar and it was not April 1st yet, and then I actually wondered if it could have been someone spoofing Even's email address.
>
> Daniel
> --
> Daniel Morissette
> Mapgears Inc
> T: +1 418-696-5056 #201
> _______________________________________________
> mapserver-dev mailing list
> [hidden email]
> https://lists.osgeo.org/mailman/listinfo/mapserver-dev
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds [was Re: Modernizing code base: C99 and maybe C++ ?]

jmckenna
Administrator
If you want my reaction, what popped into my head when reading: odd to
use email for rants or public criticizing, isn't that what Twitter is
for? ;)

-jeff



--
Jeff McKenna
MapServer Consulting and Training Services
https://gatewaygeomatics.com/



On 2019-11-27 9:55 AM, [hidden email] wrote:

> I agree, I had to do a double take myself. But I think it also points out the very real fact that if something like causes Even to rant, that this also has probably contributed to others not wanting to participate or contribute. And may also help confirm the notion for some that MapServer is old code and settled and not still a very active and worthwhile project.
>
> I think this is something we should seriously consider.
>
> Michael Smith
> US Army Corps
>
>> On Nov 27, 2019, at 8:51 AM, Daniel Morissette <[hidden email]> wrote:
>>
>> On 2019-11-27 08:38, Seth G wrote:
>>> Hope all is well - I'm not sure I've seen a rant by you before!
>>> Seth
>>
>> Ha! Ha! :-)  I've been wondering the same. I checked my calendar and it was not April 1st yet, and then I actually wondered if it could have been someone spoofing Even's email address.
>>
>> Daniel
>> --
>> Daniel Morissette
>> Mapgears Inc
>> T: +1 418-696-5056 #201


_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modernizing code base: C99 and maybe C++ ?

Seth G-2
In reply to this post by Even Rouault-2
On the C89/99 choice, I've not written enough of either to have an opinion so 1++ on whatever you think is best for the future of the project.

I think with a version 8 we should also consider:

- upping the minimum required versions of all dependencies - the page here [1] is likely out of date
- increase minimum CMake version
- increase minimum SWIG version
- drop Python2 support
- drop deprecated Mapfile keywords

There are probably several other features/options that should be considered for deprecation/dropping.

Seth

[1] https://mapserver.org/installation/unix.html#required-external-libraries

--
web:http://geographika.co.uk
twitter: @geographika

On Wed, Nov 27, 2019, at 12:43 AM, Even Rouault wrote:

> Hi,
>
> the kind of topics that is always controversial, but I do think this is a
> matter of survival for the project. I can write C89 code for sure, but which
> bad thing did I do to still be obliged to do it :-) ? Should new contributors
> be constrained by that... 2020 is just in a few days :-)
>
> Do we have a strong reason to stay with C89 for our .c files ? Not being able
> to use for loop initial declarations ( that is "for( int i = 0; i < ... ; ++)"
> ) is the example of something really annoying. Or being able to put
> declarations in the middle of a function instead of putting them at the
> beginning of it. This makes code harder to read, maintain and more error prone
> because of potential accidental reuse of variables that shouldn't.
>
> All gcc and clang compilers from the last 10 years support C99. Regarding
> MSVC, MSVC >= 2015 has decent enough support of it. Do we need to support
> older MSVC versions ? I don't think so. Neither current GDAL nor PROJ can
> built with older versions
>
> For the record, PROJ requires C99 for its few remaining .c files (and C++11
> for .cpp. So does GDAL)
>
> Next topic would be a
>
> for i in *.c; do mv $i "`basename $i .c`.cpp"; done
>
> Well, maybe not like that, because I'm expecting a ton of warnings. But more
> on a case by case basis where someone needs to do non-trivial changes in part
> of the code.
>
> Being constrained by C deeply sucks honesty. Apart from the Linux kernel which
> still resists for good reasons, I can't think of a single one for MapServer.
>
> The main point where C sucks is string manipulation, and MapServer does a ton
> of it. Being able to use std::string would be such a relief.
>
> Ah for the record, we do have C++ in the code base: mapogr.cpp and the agg
> renderer.
>
> Even
>
> --
> Spatialys - Geospatial professional services
> http://www.spatialys.com
> _______________________________________________
> mapserver-dev mailing list
> [hidden email]
> https://lists.osgeo.org/mailman/listinfo/mapserver-dev
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modernizing code base: C99 and maybe C++ ?

jmckenna
Administrator
In reply to this post by Even Rouault-2
Hi Even,

I believe we can now safely drop the C89 requirement from our codebase;
I spent quite an effort (an entire year of alpha builds) to move to a
C++11 supported compiler for MS4W builds on Windows (VisualStudio 2017).
  As MS4W version 4 embeds both Python3 and PHP7, plus of course GDAL
changed to C++11 at version 2.3.0, and PROJ 5 as well, it was mandatory
for MS4W to upgrade compilers.

I don't miss having to move variable declarations to the top of
MapServer/MapCache functions, to compile successfully, ha).

+1

-jeff




On 2019-11-26 7:43 PM, Even Rouault wrote:

> Hi,
>
> the kind of topics that is always controversial, but I do think this is a
> matter of survival for the project. I can write C89 code for sure, but which
> bad thing did I do to still be obliged to do it :-) ? Should new contributors
> be constrained by that... 2020 is just in a few days :-)
>
> Do we have a strong reason to stay with C89 for our .c files ? Not being able
> to use for loop initial declarations ( that is "for( int i = 0; i < ... ; ++)"
> ) is the example of something really annoying. Or being able to put
> declarations in the middle of a function instead of putting them at the
> beginning of it. This makes code harder to read, maintain and more error prone
> because of potential accidental reuse of variables that shouldn't.
>
> All gcc and clang compilers from the last 10 years support C99. Regarding
> MSVC, MSVC >= 2015 has decent enough support of it. Do we need to support
> older MSVC versions ? I don't think so. Neither current GDAL nor PROJ can
> built with older versions
>
> For the record, PROJ requires C99 for its few remaining .c files (and C++11
> for .cpp. So does GDAL)
>
> Next topic would be a
>
> for i in *.c; do mv $i "`basename $i .c`.cpp"; done
>
> Well, maybe not like that, because I'm expecting a ton of warnings. But more
> on a case by case basis where someone needs to do non-trivial changes in part
> of the code.
>
> Being constrained by C deeply sucks honesty. Apart from the Linux kernel which
> still resists for good reasons, I can't think of a single one for MapServer.
>
> The main point where C sucks is string manipulation, and MapServer does a ton
> of it. Being able to use std::string would be such a relief.
>
> Ah for the record, we do have C++ in the code base: mapogr.cpp and the agg
> renderer.
>
> Even
>

_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modernizing code base: C99 and maybe C++ ?

jmckenna
Administrator
In reply to this post by Even Rouault-2
By the way, I guess we'll need to make another wiki page for supported
compilers (such as https://trac.osgeo.org/gdal/wiki/SupportedCompilers).

-jeff





On 2019-11-26 7:43 PM, Even Rouault wrote:

> Hi,
>
> the kind of topics that is always controversial, but I do think this is a
> matter of survival for the project. I can write C89 code for sure, but which
> bad thing did I do to still be obliged to do it :-) ? Should new contributors
> be constrained by that... 2020 is just in a few days :-)
>
> Do we have a strong reason to stay with C89 for our .c files ? Not being able
> to use for loop initial declarations ( that is "for( int i = 0; i < ... ; ++)"
> ) is the example of something really annoying. Or being able to put
> declarations in the middle of a function instead of putting them at the
> beginning of it. This makes code harder to read, maintain and more error prone
> because of potential accidental reuse of variables that shouldn't.
>
> All gcc and clang compilers from the last 10 years support C99. Regarding
> MSVC, MSVC >= 2015 has decent enough support of it. Do we need to support
> older MSVC versions ? I don't think so. Neither current GDAL nor PROJ can
> built with older versions
>
> For the record, PROJ requires C99 for its few remaining .c files (and C++11
> for .cpp. So does GDAL)
>
> Next topic would be a
>
> for i in *.c; do mv $i "`basename $i .c`.cpp"; done
>
> Well, maybe not like that, because I'm expecting a ton of warnings. But more
> on a case by case basis where someone needs to do non-trivial changes in part
> of the code.
>
> Being constrained by C deeply sucks honesty. Apart from the Linux kernel which
> still resists for good reasons, I can't think of a single one for MapServer.
>
> The main point where C sucks is string manipulation, and MapServer does a ton
> of it. Being able to use std::string would be such a relief.
>
> Ah for the record, we do have C++ in the code base: mapogr.cpp and the agg
> renderer.
>
> Even
>


--
Jeff McKenna
MapServer Consulting and Training Services
https://gatewaygeomatics.com/
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modernizing code base: C99 and maybe C++ ?

Steve Lime-2
In reply to this post by Even Rouault-2
I don't think anyone has ranted about moving the code-base to C++ before - although Rodrigo Cabral (the original Oracle Spatial driver author) was hot on the idea years ago. Modernization is a good thing and you're probably right regarding the continuation of the project - so I'm very supportive of the idea.

Still, it's likely a lot of work depending on how far it was taken. I mean does this mean turning xxxxxObj structures into proper C++ classes, etc...? How would you see pulling off a conversion like that off? I mean, is this something that would best be handled by a single developer (not pointing fingers) under contract? If yes, how big a contact?

Definitely RFC worthy...

--Steve

On Tue, Nov 26, 2019 at 5:43 PM Even Rouault <[hidden email]> wrote:
Hi,

the kind of topics that is always controversial, but I do think this is a
matter of survival for the project. I can write C89 code for sure, but which
bad thing did I do to still be obliged to do it :-) ? Should new contributors
be constrained by that... 2020 is just in a few days :-)

Do we have a strong reason to stay with C89 for our .c files ? Not being able
to use for loop initial declarations ( that is "for( int i = 0; i < ... ; ++)"
) is the example of something really annoying. Or being able to put
declarations in the middle of a function instead of putting them at the
beginning of it. This makes code harder to read, maintain and more error prone
because of potential accidental reuse of variables that shouldn't.

All gcc and clang compilers from the last 10 years support C99. Regarding
MSVC, MSVC >= 2015 has decent enough support of it. Do we need to support
older MSVC versions ? I don't think so. Neither current GDAL nor PROJ can
built with older versions

For the record, PROJ requires C99 for its few remaining .c files (and C++11
for .cpp. So does GDAL)

Next topic would be a

for i in *.c; do mv $i "`basename $i .c`.cpp"; done

Well, maybe not like that, because I'm expecting a ton of warnings. But more
on a case by case basis where someone needs to do non-trivial changes in part
of the code.

Being constrained by C deeply sucks honesty. Apart from the Linux kernel which
still resists for good reasons, I can't think of a single one for MapServer.

The main point where C sucks is string manipulation, and MapServer does a ton
of it. Being able to use std::string would be such a relief.

Ah for the record, we do have C++ in the code base: mapogr.cpp and the agg
renderer.

Even

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev

_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modernizing code base: C99 and maybe C++ ?

Even Rouault-2
On mercredi 27 novembre 2019 11:26:10 CET Steve Lime wrote:
> I don't think anyone has ranted about moving the code-base to C++ before -

And this is exactly was really surprises me. I should
really have written that email 10 years ago (well maybe not about C++11
10 years ago :-)), I blame myself for not doing it, but everytime I touch
MapServer code base, this is the first thing that comes to mind.
Maybe I've been "spoiled" too much lately with practicing C++11 in GDAL
or PROJ, but the syntaxic sugar really lowers the cognitive burden from the
language features to the interesting algorithmic part. I can tell you that PROJ 6
would have never seen the light if it had to be written in C89 (some might
have seen that as a good outcome maybe :-)). Or I would still be chasing for
memory leaks all over the code.
Anyway as a PSC member I think it is my duty to raise the issue, be it in
a rough form. This is an "open source" project, for better and worse, so
anyone can see by themselves how the code looks like. Eh, isn't that our
main differentiating argument w.r.t proprietary software :-) ? And this
is probably what a new contributor would do.

> Still, it's likely a lot of work depending on how far it was taken. I mean
> does this mean turning xxxxxObj structures into proper C++ classes, etc...?
> How would you see pulling off a conversion like that off?

I wasn't even thinking at that. That would be a huuuuge effort. Was more
thinking about just using string manipulation facilities locally in functions.
Or using a vector, a hashmap, etc... Something incremental. Then someone bold
enough could try to adapt the internal API progressively. Even my apparent
joke about renaming the files from .c to .cpp was pretty seriously considered,
and could be seen as a positive signal, even if most of it remains mostly in C.

OK, as an example of what C could save us. This recent commit by myself, which I
needed when touching a bit the SLD code
https://github.com/mapserver/mapserver/commit/9bb379794e63e654de408cc416db692424da20e4
It is mostly my reimplemtation in C of what the std::string += operator does.
msStringConcatenate() is fundamentally an inefficient way of concatenating strings.
It has probably O(n^2) performance if you call it to aggregate lots of small substrings.

> Definitely RFC worthy...

I have written long enough RFC lately :-) Deferring on that one. The material
is pretty much above.

Even

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modernizing code base: C99 and maybe C++ ?

jratike80
In reply to this post by Even Rouault-2
Hi,

There is something that seems to touch C/C++ but not really so closely in https://trac.osgeo.org/mapserver/ticket/545.

I don't write code but what Even writes makes me believe that C++ has some true benefits. +1 if it is possible to make some useful moves  which do not require huge effort with the old codebase.

-Jukka Rahkonen-


-----Alkuperäinen viesti-----
Lähettäjä: mapserver-dev <[hidden email]> Puolesta Even Rouault
Lähetetty: keskiviikko 27. marraskuuta 2019 20.26
Vastaanottaja: Steve Lime <[hidden email]>
Kopio: [hidden email]
Aihe: Re: [mapserver-dev] Modernizing code base: C99 and maybe C++ ?

On mercredi 27 novembre 2019 11:26:10 CET Steve Lime wrote:
> I don't think anyone has ranted about moving the code-base to C++
> before -

And this is exactly was really surprises me. I should really have written that email 10 years ago (well maybe not about C++11
10 years ago :-)), I blame myself for not doing it, but everytime I touch MapServer code base, this is the first thing that comes to mind.
Maybe I've been "spoiled" too much lately with practicing C++11 in GDAL or PROJ, but the syntaxic sugar really lowers the cognitive burden from the language features to the interesting algorithmic part. I can tell you that PROJ 6 would have never seen the light if it had to be written in C89 (some might have seen that as a good outcome maybe :-)). Or I would still be chasing for memory leaks all over the code.
Anyway as a PSC member I think it is my duty to raise the issue, be it in a rough form. This is an "open source" project, for better and worse, so anyone can see by themselves how the code looks like. Eh, isn't that our main differentiating argument w.r.t proprietary software :-) ? And this is probably what a new contributor would do.

> Still, it's likely a lot of work depending on how far it was taken. I
> mean does this mean turning xxxxxObj structures into proper C++ classes, etc...?
> How would you see pulling off a conversion like that off?

I wasn't even thinking at that. That would be a huuuuge effort. Was more thinking about just using string manipulation facilities locally in functions.
Or using a vector, a hashmap, etc... Something incremental. Then someone bold enough could try to adapt the internal API progressively. Even my apparent joke about renaming the files from .c to .cpp was pretty seriously considered, and could be seen as a positive signal, even if most of it remains mostly in C.

OK, as an example of what C could save us. This recent commit by myself, which I needed when touching a bit the SLD code
https://github.com/mapserver/mapserver/commit/9bb379794e63e654de408cc416db692424da20e4
It is mostly my reimplemtation in C of what the std::string += operator does.
msStringConcatenate() is fundamentally an inefficient way of concatenating strings.
It has probably O(n^2) performance if you call it to aggregate lots of small substrings.

> Definitely RFC worthy...

I have written long enough RFC lately :-) Deferring on that one. The material is pretty much above.

Even

--
Spatialys - Geospatial professional services http://www.spatialys.com _______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modernizing code base: C99 and maybe C++ ?

Andy Colson
In reply to this post by Even Rouault-2
I'm a perl programmer at heart, I've done a little C and less C++.

I'd love to help on this project as a learning experience.  In exchange for some grunt work I'm hoping there will be code reviews that'll teach me good/bad practices.

If I can be of help, and maybe stumble around a little, please let me know.

-Andy


On 11/26/19 5:43 PM, Even Rouault wrote:

> Hi,
>
> the kind of topics that is always controversial, but I do think this is a
> matter of survival for the project. I can write C89 code for sure, but which
> bad thing did I do to still be obliged to do it :-) ? Should new contributors
> be constrained by that... 2020 is just in a few days :-)
>
> Do we have a strong reason to stay with C89 for our .c files ? Not being able
> to use for loop initial declarations ( that is "for( int i = 0; i < ... ; ++)"
> ) is the example of something really annoying. Or being able to put
> declarations in the middle of a function instead of putting them at the
> beginning of it. This makes code harder to read, maintain and more error prone
> because of potential accidental reuse of variables that shouldn't.
>
> All gcc and clang compilers from the last 10 years support C99. Regarding
> MSVC, MSVC >= 2015 has decent enough support of it. Do we need to support
> older MSVC versions ? I don't think so. Neither current GDAL nor PROJ can
> built with older versions
>
> For the record, PROJ requires C99 for its few remaining .c files (and C++11
> for .cpp. So does GDAL)
>
> Next topic would be a
>
> for i in *.c; do mv $i "`basename $i .c`.cpp"; done
>
> Well, maybe not like that, because I'm expecting a ton of warnings. But more
> on a case by case basis where someone needs to do non-trivial changes in part
> of the code.
>
> Being constrained by C deeply sucks honesty. Apart from the Linux kernel which
> still resists for good reasons, I can't think of a single one for MapServer.
>
> The main point where C sucks is string manipulation, and MapServer does a ton
> of it. Being able to use std::string would be such a relief.
>
> Ah for the record, we do have C++ in the code base: mapogr.cpp and the agg
> renderer.
>
> Even
>

_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds

Even Rouault-2
In reply to this post by Even Rouault-2
Just at the distance of pressing the "Merge" button:
https://github.com/mapserver/mapserver/pull/5935

The diff stat is pretty eloquent about the benefits:
 54 files changed, 118 insertions(+), 1298 deletions(-)

Said otherwise, 0.7% of the total number of lines of .c/.cpp code gone.

I didn't even realize there were so many #ifdef before removing them :-) !

Even

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds

Stephen Woodbridge-4
On 11/29/2019 10:21 AM, Even Rouault wrote:

> Just at the distance of pressing the "Merge" button:
> https://github.com/mapserver/mapserver/pull/5935
>
> The diff stat is pretty eloquent about the benefits:
>   54 files changed, 118 insertions(+), 1298 deletions(-)
>
> Said otherwise, 0.7% of the total number of lines of .c/.cpp code gone.
>
> I didn't even realize there were so many #ifdef before removing them :-) !
>
> Even
>
Wow! nice work. I'm all for simplification of the code. And a non-voting
+1 on modernizing the code base and moving to C++. I'm impressed by all
the improvements and enhancements you do on so many projects and I'm for
things that make your life easier and improve the code. I've seen old
legacy code way down and smother other projects and would hate to see
that happen on mapserver.

Thanks!
  -Steve W
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds

Tom Kralidis
+1! Code deletions are often more valuable!

..Tom

> On Nov 29, 2019, at 10:33, Stephen Woodbridge <[hidden email]> wrote:
>
> On 11/29/2019 10:21 AM, Even Rouault wrote:
>> Just at the distance of pressing the "Merge" button:
>> https://github.com/mapserver/mapserver/pull/5935
>>
>> The diff stat is pretty eloquent about the benefits:
>>  54 files changed, 118 insertions(+), 1298 deletions(-)
>>
>> Said otherwise, 0.7% of the total number of lines of .c/.cpp code gone.
>>
>> I didn't even realize there were so many #ifdef before removing them :-) !
>>
>> Even
>>
> Wow! nice work. I'm all for simplification of the code. And a non-voting +1 on modernizing the code base and moving to C++. I'm impressed by all the improvements and enhancements you do on so many projects and I'm for things that make your life easier and improve the code. I've seen old legacy code way down and smother other projects and would hate to see that happen on mapserver.
>
> Thanks!
>  -Steve W
> _______________________________________________
> mapserver-dev mailing list
> [hidden email]
> https://lists.osgeo.org/mailman/listinfo/mapserver-dev
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds

jmckenna
Administrator
In reply to this post by Even Rouault-2
Regarding users compiling this, how would they know that GDAL+PROJ are
required, if we remove WITH-GDAL and WITH-PROJ from the cmake syntax?
(those cmake hints are a lifeline for compiling software I find).

-jeff



On 2019-11-29 11:21 AM, Even Rouault wrote:

> Just at the distance of pressing the "Merge" button:
> https://github.com/mapserver/mapserver/pull/5935
>
> The diff stat is pretty eloquent about the benefits:
>   54 files changed, 118 insertions(+), 1298 deletions(-)
>
> Said otherwise, 0.7% of the total number of lines of .c/.cpp code gone.
>
> I didn't even realize there were so many #ifdef before removing them :-) !
>
> Even
>
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds

Even Rouault-2
On vendredi 29 novembre 2019 12:02:12 CET Jeff McKenna wrote:
> Regarding users compiling this, how would they know that GDAL+PROJ are
> required, if we remove WITH-GDAL and WITH-PROJ from the cmake syntax?
> (those cmake hints are a lifeline for compiling software I find).

Exactly as they would figure out for other required dependencies we have
already (libpng, libjpeg, libfreetype). cmake will loudly and clearly complain
about what's wrong:

-- Could NOT find PROJ (missing:  PROJ_LIBRARY)
CMake Error at CMakeLists.txt:79 (message):
  PROJ library/component could not be found and is a mandatory dependency

    HINT:
    - add the PROJ install directory to the CMAKE_PREFIX_PATH variable (-
DCMAKE_PREFIX_PATH="/path/to/PROJ-install-dir;/path/to/other/dirs"
Call Stack (most recent call first):
  CMakeLists.txt:390 (report_mandatory_not_found)

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dropping support for non-GDAL and non-PROJ builds

jmckenna
Administrator
On 2019-11-29 12:13 PM, Even Rouault wrote:

> On vendredi 29 novembre 2019 12:02:12 CET Jeff McKenna wrote:
>> Regarding users compiling this, how would they know that GDAL+PROJ are
>> required, if we remove WITH-GDAL and WITH-PROJ from the cmake syntax?
>> (those cmake hints are a lifeline for compiling software I find).
>
> Exactly as they would figure out for other required dependencies we have
> already (libpng, libjpeg, libfreetype). cmake will loudly and clearly complain
> about what's wrong:
>
> -- Could NOT find PROJ (missing:  PROJ_LIBRARY)
> CMake Error at CMakeLists.txt:79 (message):
>    PROJ library/component could not be found and is a mandatory dependency
>
>      HINT:
>      - add the PROJ install directory to the CMAKE_PREFIX_PATH variable (-
> DCMAKE_PREFIX_PATH="/path/to/PROJ-install-dir;/path/to/other/dirs"
> Call Stack (most recent call first):
>    CMakeLists.txt:390 (report_mandatory_not_found)
>

I meant besides battling through cmake errors.  cmake command has a
switch "-LA" that lists all available options (WITH_PHP, WITH_PIXMAN,
etc) that is very useful when compiling a new software, and learning of
all its dependencies.  But it sounds like there is no way to tell users
of these hidden requirements (at least not from my googling).

-jeff

_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
Reply | Threaded
Open this post in threaded view
|

Re: Modernizing code base: C99 and maybe C++ ?

Even Rouault-2
In reply to this post by Seth G-2
On mercredi 27 novembre 2019 15:15:45 CET Seth G wrote:
> On the C89/99 choice, I've not written enough of either to have an opinion
> so 1++ on whatever you think is best for the future of the project.

I've just had a quick try on mapogcfiltercommon.c (now .cpp) in
https://github.com/rouault/mapserver/commit/7208025a897ef4095886ec80153f00409ac73cee#diff-ec6ec0373f1c666c1950903a0ba12b45

I believe everyone, developer or not, can appreciate how the += operator makes the code:
1. smaller : += is more compact than msStringConcatenate()
2. clearer: our limited brain can concentrate more easily on the interesting high level logic.
3. and ... more secure. No need to make sure the buffer will not overflow,
   that it is nul terminated, that we increment properly iTmp, etc.

Actually this example is not randomly chosen. This would have avoided a past
vulnerability that we had to patch with
https://github.com/mapserver/mapserver/commit/e52a436c0e1c5e9f7ef13428dba83194a800f4df

(I'm not saying use of C++ makes you security immune automatically. Certainly not)

This also demonstrates this can be done in an incremental fashion, at
least sometimes...

> I think with a version 8 we should also consider:
>
> - upping the minimum required versions of all dependencies -
-  the page here
> [1] is likely out of date - increase minimum CMake version
> - increase minimum SWIG version
> - drop Python2 support

One way of addressing this would be to determine for example which version of
popular Linux distributions we want to support, look at the version of the
dependencies they support, and use that as our baseline.

Hint: CentOS/RHEL 7 would certainly to be the most constraining factor if we
aim at supporting "old-stable" releases. I see for example the EPEL 7 repo only ships
with GDAL 1.11 whereas we now require GDAL 2 for master.

Even

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
mapserver-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/mapserver-dev
12