[gdal-dev] Proposed updates to the GDAL feature style spec

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[gdal-dev] Proposed updates to the GDAL feature style spec

Alan Thomas
Hello all,

In updating the PDF vector feature writer, it became apparent that
there are some deficiencies in the GDAL feature style specification
document (http://www.gdal.org/ogr_feature_style.html). I thought this
would be a good opportunity to update the document in other ways as
well.

See the diff of the proposed changes:
https://github.com/ThinkSpatial/gdal/commit/dc212edfa9872779c94d4534bdfeecb4cfbf3aff#diff-8bfbe5b06a25ed97077e344069647884

I've made a large number of editorial and non-substantive changes to
make the spec easier to read. The proposed substantive changes are:

--

1. Remove the "DRAFT" notice at the top.

Rationale: The spec has seen few updates in the last 10 years, so it
seems unnecessary to call it a draft.

2. Remove some unnecessary prognostications about things that may be
added in future versions of the specification, like predefined color
names, more brush patterns, or the "gp" parameter for pens.

Rationale: The document is no longer a discussion paper but an
actively used specification. The string "gp" does not appear anywhere
in GDAL code.

3. Remove information that contradicts reality, such as "A dataset can
have a default style that applies to all features" (there is no such
capability) and the statement that OGRFeature::GetStyleString()
handles style table lookups (it does not).

4. Replace the wording "An empty style string means that the feature
directly inherits its style
from the layer it is in" with "An empty style string means that the
feature's style is unspecified".

Rationale: OGR layers do not have a style. Even if they did, the spec
doesn't say what would happen if the layer also lacked a style.

5. Clarify the usage of BRUSH fc and bc; change the definition of
ogr-brush-1 to mean a solid fill in the selected background color;
change the suggested default for BRUSH bc to transparent (#FFFFFF00).

Rationale: The spec is not explicit on how fc and bc are meant to
work. A true null brush seems unnecessary, as the BRUSH() style tool
can simply be omitted. If a null brush really is required, ogr-brush-1
will continue to act as a null brush by default, assuming software
uses a fully transparent color as a default for bc.

6. Explain more clearly what was intended by the SYMBOL o (outline
color) parameter.

7. Provide notes explaining the subtleties of font size and text anchor values.

Rationale: See <https://trac.osgeo.org/gdal/ticket/7185#comment:2>.
This is option 1, "Admit defeat", but leaving the door open to dealing
with the problem properly at a later date if necessary.

8. Clarify that backslash characters must be escaped in text values.

Rationale: Without this, it is impossible to indicate a text string
that ends with a backslash.

9. Clarify that a 0 or 1 value is expected after Boolean parameters
(bo, it, un, st).

Rationale: Contradictory usage in existing drivers (MITAB) vs
OGRStyleTool class.

10. Fix incorrect text stretch example (a stretch of 150 is 150% of
the usual width, not 150% wider).

11. Update sections 2.7 and 3 to match reality.

--

Please speak up if you have concerns about these changes, or if you
have suggestions for additional changes that could be made at the same
time.

Alan

--
Alan Thomas
Software Developer
ThinkSpatial
http://www.thinkspatial.com.au
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Proposed updates to the GDAL feature style spec

Even Rouault-2

Alan,

 

Thanks a lot for this refresh ! I've just a single remark

 

> 5. [...] change the definition of

> ogr-brush-1 to mean a solid fill in the selected background color;

> change the suggested default for BRUSH bc to transparent (#FFFFFF00).

>

> Rationale: The spec is not explicit on how fc and bc are meant to

> work. A true null brush seems unnecessary, as the BRUSH() style tool

> can simply be omitted. If a null brush really is required, ogr-brush-1

> will continue to act as a null brush by default, assuming software

> uses a fully transparent color as a default for bc.

 

I'd suggest that we add a rule so as to keep the current semantics: if ogr-brush-1 is used, then bc must be omitted (or set to a color with alpha=0). It would be confusing to have the possibility to have 2 ways to express a solid fill with either ogr-brush-0 + fc or ogr-brush-1 + bc. So people wanting a solid fill should use ogr-brush-0 (or don't specify it)

 

For example MapServer (likely the most comprehensive user of OGR feature style) has a hard-coded behaviour when encountering ogr-brush-1 where it completely ignores bc:

https://github.com/mapserver/mapserver/blob/branch-7-0/mapogr.cpp#L5132

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


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

Re: Proposed updates to the GDAL feature style spec

Alan Thomas
Thanks for the feedback, Even. If there is no other feedback in the
next few days I will merge the change, as well as an extra code fix to
implement change 8.

On 2 January 2018 at 21:47, Even Rouault <[hidden email]> wrote:

>> 5. [...] change the definition of
>> ogr-brush-1 to mean a solid fill in the selected background color;
>> change the suggested default for BRUSH bc to transparent
>> (#FFFFFF00).
>
> I'd suggest that we add a rule so as to keep the current semantics: if
> ogr-brush-1 is used, then bc must be omitted (or set to a color with
> alpha=0). It would be confusing to have the possibility to have 2 ways to
> express a solid fill with either ogr-brush-0 + fc or ogr-brush-1 + bc. So
> people wanting a solid fill should use ogr-brush-0 (or don't specify it)
>

I expected that this might be a problematic change. I'm happy to leave
ogr-brush-1 alone. If existing code is already special-casing
ogr-brush-1, then keeping the spec the way it is would be less
confusing.

Therefore line 492 will say "null brush (transparent - no fill,
irrespective of fc or bc values)".

Alan

--
Alan Thomas
Software Developer
ThinkSpatial
http://www.thinkspatial.com.au
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Proposed updates to the GDAL feature style spec

Frank Warmerdam
Alan,

I'm also supportive of your change while not having a strong opinion
on the ogr-brush-1 issue.  Thanks for showing some love to feature
styles *including* the specification.

Best regards,
Frank


On Wed, Jan 3, 2018 at 12:52 AM, Alan Thomas
<[hidden email]> wrote:

> Thanks for the feedback, Even. If there is no other feedback in the
> next few days I will merge the change, as well as an extra code fix to
> implement change 8.
>
> On 2 January 2018 at 21:47, Even Rouault <[hidden email]> wrote:
>>> 5. [...] change the definition of
>>> ogr-brush-1 to mean a solid fill in the selected background color;
>>> change the suggested default for BRUSH bc to transparent
>>> (#FFFFFF00).
>>
>> I'd suggest that we add a rule so as to keep the current semantics: if
>> ogr-brush-1 is used, then bc must be omitted (or set to a color with
>> alpha=0). It would be confusing to have the possibility to have 2 ways to
>> express a solid fill with either ogr-brush-0 + fc or ogr-brush-1 + bc. So
>> people wanting a solid fill should use ogr-brush-0 (or don't specify it)
>>
>
> I expected that this might be a problematic change. I'm happy to leave
> ogr-brush-1 alone. If existing code is already special-casing
> ogr-brush-1, then keeping the spec the way it is would be less
> confusing.
>
> Therefore line 492 will say "null brush (transparent - no fill,
> irrespective of fc or bc values)".
>
> Alan
>
> --
> Alan Thomas
> Software Developer
> ThinkSpatial
> http://www.thinkspatial.com.au
> _______________________________________________
> gdal-dev mailing list
> [hidden email]
> https://lists.osgeo.org/mailman/listinfo/gdal-dev



--
---------------------------------------+--------------------------------------
I set the clouds in motion - turn up   | Frank Warmerdam, [hidden email]
light and sound - activate the windows |
and watch the world go round - Rush    | Geospatial Software Developer
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

[gdal-dev] CPLJSONDocument

Dmitry Baryshnikov-2
Hi everybody,

Happy new year and lot of success in 2018!

I would like to discuss my pull request
https://github.com/OSGeo/gdal/pull/282

I created a thin wrapper around the json-c library which wide using in GDAL.

This is C++ interface which hides C memory management and provides nice
API. The web or disk json documents reading chunk by chunk with progress
indication also added.

In future, the json-c can be easily switch to something other without
breaking the existing code.

The CPLJSONDocument/CPLJSONObject/CPLJSONArray usage examples can be
found in frmts/pds driver and c++ unit test in autotest/cpp/test_cpl.cpp.

Is this ready to merge into the trunk? Any objections?

Best regards,
     Dmitry


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

Re: CPLJSONDocument

Even Rouault-2

Dmitry,

 

> Is this ready to merge into the trunk? Any objections?

 

I did some review of your code. If nobody has extra remarks in the next days, just merge it.

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


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

Re: CPLJSONDocument

Sean Gillies-3
In reply to this post by Dmitry Baryshnikov-2
Hi Dmitry,

I scanned the PR and it seems reasonable to me. I'm barely a C++ programmer at all and it's clear to me, more clear than before. That said, I'm not a fan of wrapping things that could be replaced. Have you looked into whether a more performant C++ JSON library could be used? I haven't run the benchmarks, but json-c compares pretty poorly to others in https://github.com/miloyip/nativejson-benchmark.


On Wed, Jan 3, 2018 at 12:04 PM, Dmitry Baryshnikov <[hidden email]> wrote:
Hi everybody,

Happy new year and lot of success in 2018!

I would like to discuss my pull request https://github.com/OSGeo/gdal/pull/282

I created a thin wrapper around the json-c library which wide using in GDAL.

This is C++ interface which hides C memory management and provides nice API. The web or disk json documents reading chunk by chunk with progress indication also added.

In future, the json-c can be easily switch to something other without breaking the existing code.

The CPLJSONDocument/CPLJSONObject/CPLJSONArray usage examples can be found in frmts/pds driver and c++ unit test in autotest/cpp/test_cpl.cpp.

Is this ready to merge into the trunk? Any objections?

Best regards,
    Dmitry


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev



--
Sean Gillies

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

Re: CPLJSONDocument

Kurt Schwehr-2
+1 for wrapping the old C code in some cleaner abstractions!

But +10 for switching to a from the ground up C++ JSON library unless there are clear reasons for a core C library (I don't think there are)

If we are talking about this kind of code, there are several things that have bugged me in general about GDAL for a long time.

* Passing char *psz yada all over the place in pure C++ code.  A const std::string is usually not a noticeable expense and is a lot safer
* CPLString when std::string will do just fine.  And we can write free functions to operate on strings.  I'm generally bothered by subclassing of std::string as CPLString.  After reading large amounts of C++ code, I think it adds more confusion than it ever helps over having clean free functions.  Interop and analysis with CPLString's is no fun.

-kurt

On Fri, Jan 5, 2018 at 7:44 AM, Sean Gillies <[hidden email]> wrote:
Hi Dmitry,

I scanned the PR and it seems reasonable to me. I'm barely a C++ programmer at all and it's clear to me, more clear than before. That said, I'm not a fan of wrapping things that could be replaced. Have you looked into whether a more performant C++ JSON library could be used? I haven't run the benchmarks, but json-c compares pretty poorly to others in https://github.com/miloyip/nativejson-benchmark.


On Wed, Jan 3, 2018 at 12:04 PM, Dmitry Baryshnikov <[hidden email]> wrote:
Hi everybody,

Happy new year and lot of success in 2018!

I would like to discuss my pull request https://github.com/OSGeo/gdal/pull/282

I created a thin wrapper around the json-c library which wide using in GDAL.

This is C++ interface which hides C memory management and provides nice API. The web or disk json documents reading chunk by chunk with progress indication also added.

In future, the json-c can be easily switch to something other without breaking the existing code.

The CPLJSONDocument/CPLJSONObject/CPLJSONArray usage examples can be found in frmts/pds driver and c++ unit test in autotest/cpp/test_cpl.cpp.

Is this ready to merge into the trunk? Any objections?

Best regards,
    Dmitry


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev



--
Sean Gillies

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev



--

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

Re: CPLJSONDocument

Kurt Schwehr-2
I think the more important factors than speed for a C++ lib are: security, stability, maintainability, and memory usage (low stack usage and constrainable heap).

I really don't want to have us go through yet more piles of fuzzer bugs for a library we depend on.  It would be nice to have that already done well by the lib.

On Fri, Jan 5, 2018 at 7:54 AM, Kurt Schwehr <[hidden email]> wrote:
+1 for wrapping the old C code in some cleaner abstractions!

But +10 for switching to a from the ground up C++ JSON library unless there are clear reasons for a core C library (I don't think there are)

If we are talking about this kind of code, there are several things that have bugged me in general about GDAL for a long time.

* Passing char *psz yada all over the place in pure C++ code.  A const std::string is usually not a noticeable expense and is a lot safer
* CPLString when std::string will do just fine.  And we can write free functions to operate on strings.  I'm generally bothered by subclassing of std::string as CPLString.  After reading large amounts of C++ code, I think it adds more confusion than it ever helps over having clean free functions.  Interop and analysis with CPLString's is no fun.

-kurt

On Fri, Jan 5, 2018 at 7:44 AM, Sean Gillies <[hidden email]> wrote:
Hi Dmitry,

I scanned the PR and it seems reasonable to me. I'm barely a C++ programmer at all and it's clear to me, more clear than before. That said, I'm not a fan of wrapping things that could be replaced. Have you looked into whether a more performant C++ JSON library could be used? I haven't run the benchmarks, but json-c compares pretty poorly to others in https://github.com/miloyip/nativejson-benchmark.


On Wed, Jan 3, 2018 at 12:04 PM, Dmitry Baryshnikov <[hidden email]> wrote:
Hi everybody,

Happy new year and lot of success in 2018!

I would like to discuss my pull request https://github.com/OSGeo/gdal/pull/282

I created a thin wrapper around the json-c library which wide using in GDAL.

This is C++ interface which hides C memory management and provides nice API. The web or disk json documents reading chunk by chunk with progress indication also added.

In future, the json-c can be easily switch to something other without breaking the existing code.

The CPLJSONDocument/CPLJSONObject/CPLJSONArray usage examples can be found in frmts/pds driver and c++ unit test in autotest/cpp/test_cpl.cpp.

Is this ready to merge into the trunk? Any objections?

Best regards,
    Dmitry


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

Re: CPLJSONDocument

Dmitry Baryshnikov-2
Hi Sean,

First of all I agreed with Kurt. Json-c is GDAL internal library and had
inspected by fuzzer.

Also json-c widely used in GDAL:

gdal/gcore
gdal/ogr/ogrsf_frmts/carto
gdal/ogr/ogrsf_frmts/amigocloud
gdal/ogr/ogrsf_frmts/cloudant
gdal/ogr/ogrsf_frmts/plscenes
gdal/ogr/ogrsf_frmts/geojson
gdal/ogr/ogrsf_frmts/gmlas
gdal/ogr/ogrsf_frmts/elastic
gdal/ogr/ogrsf_frmts/couchdb
gdal/ogr
gdal/frmts/rda
gdal/frmts/arg
gdal/frmts/mbtiles
gdal/frmts/plmosaic
gdal/frmts/pds

It's big work to replace json-c in all code smoothly.

Best regards,
     Dmitry

05.01.2018 19:05, Kurt Schwehr пишет:

> I think the more important factors than speed for a C++ lib are: security,
> stability, maintainability, and memory usage (low stack usage and
> constrainable heap).
>
> I really don't want to have us go through yet more piles of fuzzer bugs for
> a library we depend on.  It would be nice to have that already done well by
> the lib.
>
> On Fri, Jan 5, 2018 at 7:54 AM, Kurt Schwehr <[hidden email]> wrote:
>
>> +1 for wrapping the old C code in some cleaner abstractions!
>>
>> But +10 for switching to a from the ground up C++ JSON library unless
>> there are clear reasons for a core C library (I don't think there are)
>>
>> If we are talking about this kind of code, there are several things that
>> have bugged me in general about GDAL for a long time.
>>
>> * Passing char *psz yada all over the place in pure C++ code.  A const
>> std::string is usually not a noticeable expense and is a lot safer
>> * CPLString when std::string will do just fine.  And we can write free
>> functions to operate on strings.  I'm generally bothered by subclassing of
>> std::string as CPLString.  After reading large amounts of C++ code, I think
>> it adds more confusion than it ever helps over having clean free
>> functions.  Interop and analysis with CPLString's is no fun.
>>    https://stackoverflow.com/questions/6006860/why-should-
>> one-not-derive-from-c-std-string-class
>>
>> -kurt
>>
>> On Fri, Jan 5, 2018 at 7:44 AM, Sean Gillies <[hidden email]> wrote:
>>
>>> Hi Dmitry,
>>>
>>> I scanned the PR and it seems reasonable to me. I'm barely a C++
>>> programmer at all and it's clear to me, more clear than before. That said,
>>> I'm not a fan of wrapping things that could be replaced. Have you looked
>>> into whether a more performant C++ JSON library could be used? I haven't
>>> run the benchmarks, but json-c compares pretty poorly to others in
>>> https://github.com/miloyip/nativejson-benchmark.
>>>
>>>
>>> On Wed, Jan 3, 2018 at 12:04 PM, Dmitry Baryshnikov <[hidden email]
>>>> wrote:
>>>> Hi everybody,
>>>>
>>>> Happy new year and lot of success in 2018!
>>>>
>>>> I would like to discuss my pull request https://github.com/OSGeo/gdal/
>>>> pull/282
>>>>
>>>> I created a thin wrapper around the json-c library which wide using in
>>>> GDAL.
>>>>
>>>> This is C++ interface which hides C memory management and provides nice
>>>> API. The web or disk json documents reading chunk by chunk with progress
>>>> indication also added.
>>>>
>>>> In future, the json-c can be easily switch to something other without
>>>> breaking the existing code.
>>>>
>>>> The CPLJSONDocument/CPLJSONObject/CPLJSONArray usage examples can be
>>>> found in frmts/pds driver and c++ unit test in autotest/cpp/test_cpl.cpp.
>>>>
>>>> Is this ready to merge into the trunk? Any objections?
>>>>
>>>> Best regards,
>>>>      Dmitry
>>>>
>>>>

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

Re: CPLJSONDocument

Dmitry Baryshnikov-2
In reply to this post by Kurt Schwehr-2

Hi Kurt,

Can you explain what should be done in PR?

Do you mean to replace all const char* to?

1. const char* -> const CPLString &

const char *GetString(const char *pszName, const char *pszDefault = "") const; ->

CPLString GetString(const CPLString &soName, const CPLString &soDefault = "") const;

or

2. const char* -> const std::string &

const char *GetString(const char *pszName, const char *pszDefault = "") const; ->

std::string GetString(const std::string &soName, const std::string &soDefault = "") const;

or?

Best regards,
    Dmitry
05.01.2018 18:54, Kurt Schwehr пишет:
+1 for wrapping the old C code in some cleaner abstractions!

But +10 for switching to a from the ground up C++ JSON library unless there
are clear reasons for a core C library (I don't think there are)

If we are talking about this kind of code, there are several things that
have bugged me in general about GDAL for a long time.

* Passing char *psz yada all over the place in pure C++ code.  A const
std::string is usually not a noticeable expense and is a lot safer
* CPLString when std::string will do just fine.  And we can write free
functions to operate on strings.  I'm generally bothered by subclassing of
std::string as CPLString.  After reading large amounts of C++ code, I think
it adds more confusion than it ever helps over having clean free
functions.  Interop and analysis with CPLString's is no fun.

https://stackoverflow.com/questions/6006860/why-should-one-not-derive-from-c-std-string-class

-kurt

On Fri, Jan 5, 2018 at 7:44 AM, Sean Gillies [hidden email] wrote:

Hi Dmitry,

I scanned the PR and it seems reasonable to me. I'm barely a C++
programmer at all and it's clear to me, more clear than before. That said,
I'm not a fan of wrapping things that could be replaced. Have you looked
into whether a more performant C++ JSON library could be used? I haven't
run the benchmarks, but json-c compares pretty poorly to others in
https://github.com/miloyip/nativejson-benchmark.


On Wed, Jan 3, 2018 at 12:04 PM, Dmitry Baryshnikov [hidden email]
wrote:

Hi everybody,

Happy new year and lot of success in 2018!

I would like to discuss my pull request https://github.com/OSGeo/gdal/
pull/282

I created a thin wrapper around the json-c library which wide using in
GDAL.

This is C++ interface which hides C memory management and provides nice
API. The web or disk json documents reading chunk by chunk with progress
indication also added.

In future, the json-c can be easily switch to something other without
breaking the existing code.

The CPLJSONDocument/CPLJSONObject/CPLJSONArray usage examples can be
found in frmts/pds driver and c++ unit test in autotest/cpp/test_cpl.cpp.

Is this ready to merge into the trunk? Any objections?

Best regards,
    Dmitry


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev



--
Sean Gillies

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev





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

Re: CPLJSONDocument

Kurt Schwehr-2
My preference (and not speaking for the gdal community) for C++ classes would be:

1. replace const char * -> const std::string &
2. replace CPLString -> std::string

std::string GetString(const std::string &soName, const std::string &soDefault = "") const;

This is where it would be good to get input from others.

I base the above on maximizing safety while trying to let the compiler do its best job at optimizing.  Then in about 2022, we can see about std::string_view :(

On Fri, Jan 5, 2018 at 12:26 PM, Dmitry Baryshnikov <[hidden email]> wrote:

Hi Kurt,

Can you explain what should be done in PR?

Do you mean to replace all const char* to?

1. const char* -> const CPLString &

const char *GetString(const char *pszName, const char *pszDefault = "") const; ->

CPLString GetString(const CPLString &soName, const CPLString &soDefault = "") const;

or

2. const char* -> const std::string &

const char *GetString(const char *pszName, const char *pszDefault = "") const; ->

std::string GetString(const std::string &soName, const std::string &soDefault = "") const;

or?

Best regards,
    Dmitry
05.01.2018 18:54, Kurt Schwehr пишет:
+1 for wrapping the old C code in some cleaner abstractions!

But +10 for switching to a from the ground up C++ JSON library unless there
are clear reasons for a core C library (I don't think there are)

If we are talking about this kind of code, there are several things that
have bugged me in general about GDAL for a long time.

* Passing char *psz yada all over the place in pure C++ code.  A const
std::string is usually not a noticeable expense and is a lot safer
* CPLString when std::string will do just fine.  And we can write free
functions to operate on strings.  I'm generally bothered by subclassing of
std::string as CPLString.  After reading large amounts of C++ code, I think
it adds more confusion than it ever helps over having clean free
functions.  Interop and analysis with CPLString's is no fun.

https://stackoverflow.com/questions/6006860/why-should-one-not-derive-from-c-std-string-class

-kurt

On Fri, Jan 5, 2018 at 7:44 AM, Sean Gillies [hidden email] wrote:

Hi Dmitry,

I scanned the PR and it seems reasonable to me. I'm barely a C++
programmer at all and it's clear to me, more clear than before. That said,
I'm not a fan of wrapping things that could be replaced. Have you looked
into whether a more performant C++ JSON library could be used? I haven't
run the benchmarks, but json-c compares pretty poorly to others in
https://github.com/miloyip/nativejson-benchmark.


On Wed, Jan 3, 2018 at 12:04 PM, Dmitry Baryshnikov [hidden email]
wrote:

Hi everybody,

Happy new year and lot of success in 2018!

I would like to discuss my pull request https://github.com/OSGeo/gdal/
pull/282

I created a thin wrapper around the json-c library which wide using in
GDAL.

This is C++ interface which hides C memory management and provides nice
API. The web or disk json documents reading chunk by chunk with progress
indication also added.

In future, the json-c can be easily switch to something other without
breaking the existing code.

The CPLJSONDocument/CPLJSONObject/CPLJSONArray usage examples can be
found in frmts/pds driver and c++ unit test in autotest/cpp/test_cpl.cpp.

Is this ready to merge into the trunk? Any objections?

Best regards,
    Dmitry


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev


--
Sean Gillies

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev






--

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

Re: CPLJSONDocument

Mateusz Loskot
On 5 January 2018 at 21:43, Kurt Schwehr <[hidden email]> wrote:
> My preference (and not speaking for the gdal community) for C++ classes
> would be:
>
> 1. replace const char * -> const std::string &
> 2. replace CPLString -> std::string

+1

> Then in about 2022, we can see about std::string_view :(

+1

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: CPLJSONDocument

Andrew C Aitchison-2
In reply to this post by Kurt Schwehr-2
On Fri, 5 Jan 2018, Kurt Schwehr wrote:

> * Passing char *psz yada all over the place in pure C++ code.  A const
> std::string is usually not a noticeable expense and is a lot safer
> * CPLString when std::string will do just fine.  And we can write free
> functions to operate on strings.  I'm generally bothered by subclassing of
> std::string as CPLString.  After reading large amounts of C++ code, I think
> it adds more confusion than it ever helps over having clean free
> functions.  Interop and analysis with CPLString's is no fun.
>
> https://stackoverflow.com/questions/6006860/why-should-one-not-derive-from-c-std-string-class

Can you point me to some good examples of good pure C++ code in GDAL
- ideally driver code ? The driver tutorial
   http://www.gdal.org/gdal_drivertut.html
is full of pszFilename and other psz variables.

When I taught myself C++ in 1992 std::string did not exist,
and my gdal work is my only C++ practice since 1993.
In my exploration of gdal code I have seen very little use of std::string
and plenty of CPLString; it would be a great help to have good examples to
copy.

--
Andrew C. Aitchison Cambridge, UK
  [hidden email]


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

Re: CPLJSONDocument

Kurt Schwehr-2
Andrew,

My take:

GDAL is a not a place to look for good modern C++ code.  And there is a of old style C++ baked into the APIs, so drivers are particularly tough.

You can look for use of unique_ptr for some code that is heading in that direction.

The other general problem with C++ code in GDAL is that it wasn't designed with C++ testing in mind.  I've broken headers out of some of the drivers so I can skip past the GDALOpen/Identify interfaces and test inside the member functions.

You can look through https://github.com/schwehr/gdal-autotest2/tree/master/cpp but most of that code is pretty different as it is mostly tests.

GEOS is starting to modernize so you can look some there, but also isn't particularly strong C++11 (yet)

A good place to start is with project style guides.  There are many around with different takes.  The one I work to is:




On Sat, Jan 6, 2018 at 3:58 AM, Andrew C Aitchison <[hidden email]> wrote:
On Fri, 5 Jan 2018, Kurt Schwehr wrote:

* Passing char *psz yada all over the place in pure C++ code.  A const
std::string is usually not a noticeable expense and is a lot safer
* CPLString when std::string will do just fine.  And we can write free
functions to operate on strings.  I'm generally bothered by subclassing of
std::string as CPLString.  After reading large amounts of C++ code, I think
it adds more confusion than it ever helps over having clean free
functions.  Interop and analysis with CPLString's is no fun.

https://stackoverflow.com/questions/6006860/why-should-one-not-derive-from-c-std-string-class

Can you point me to some good examples of good pure C++ code in GDAL
- ideally driver code ? The driver tutorial
  http://www.gdal.org/gdal_drivertut.html
is full of pszFilename and other psz variables.

When I taught myself C++ in 1992 std::string did not exist,
and my gdal work is my only C++ practice since 1993.
In my exploration of gdal code I have seen very little use of std::string
and plenty of CPLString; it would be a great help to have good examples to copy.

--
Andrew C. Aitchison                                     Cambridge, UK
                        [hidden email]





--

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

Re: CPLJSONDocument

Dmitry Baryshnikov-2
In reply to this post by Kurt Schwehr-2

Hi,

I changed CPLJSONObject/CPLJSONArray  methods string parameters to std::string and merged last trunk changes.

Still leave 3 methods with const char* to simplify code because according to standard conversion sequence, methods with input (const char*, const char*) parameters executes method with signature (std:;string&, bool):

Add("key", "value") --> Add(std::string osName, bool bValue)
Set("key", "value") --> Set(std::string osName, bool bValue)

See notes in pull request (https://github.com/OSGeo/gdal/pull/282#issuecomment-355766795).
I'm ready to merge current pull request into the trunk.
Best regards,
    Dmitry
05.01.2018 23:43, Kurt Schwehr пишет:
My preference (and not speaking for the gdal community) for C++ classes
would be:

1. replace const char * -> const std::string &
2. replace CPLString -> std::string

std::string GetString(const std::string &soName, const std::string
&soDefault = "") const;

This is where it would be good to get input from others.

I base the above on maximizing safety while trying to let the compiler do
its best job at optimizing.  Then in about 2022, we can see about
std::string_view :(

On Fri, Jan 5, 2018 at 12:26 PM, Dmitry Baryshnikov [hidden email]
wrote:

Hi Kurt,

Can you explain what should be done in PR?

Do you mean to replace all const char* to?

1. const char* -> const CPLString &

const char *GetString(const char *pszName, const char *pszDefault = "")
const; ->

CPLString GetString(const CPLString &soName, const CPLString &soDefault =
"") const;

or

2. const char* -> const std::string &

const char *GetString(const char *pszName, const char *pszDefault = "")
const; ->

std::string GetString(const std::string &soName, const std::string
&soDefault = "") const;

or?

Best regards,
    Dmitry

05.01.2018 18:54, Kurt Schwehr пишет:

+1 for wrapping the old C code in some cleaner abstractions!

But +10 for switching to a from the ground up C++ JSON library unless there
are clear reasons for a core C library (I don't think there are)

If we are talking about this kind of code, there are several things that
have bugged me in general about GDAL for a long time.

* Passing char *psz yada all over the place in pure C++ code.  A const
std::string is usually not a noticeable expense and is a lot safer
* CPLString when std::string will do just fine.  And we can write free
functions to operate on strings.  I'm generally bothered by subclassing of
std::string as CPLString.  After reading large amounts of C++ code, I think
it adds more confusion than it ever helps over having clean free
functions.  Interop and analysis with CPLString's is no fun.
https://stackoverflow.com/questions/6006860/why-should-one-not-derive-from-c-std-string-class

-kurt

On Fri, Jan 5, 2018 at 7:44 AM, Sean Gillies [hidden email] [hidden email] wrote:


Hi Dmitry,

I scanned the PR and it seems reasonable to me. I'm barely a C++
programmer at all and it's clear to me, more clear than before. That said,
I'm not a fan of wrapping things that could be replaced. Have you looked
into whether a more performant C++ JSON library could be used? I haven't
run the benchmarks, but json-c compares pretty poorly to others inhttps://github.com/miloyip/nativejson-benchmark.


On Wed, Jan 3, 2018 at 12:04 PM, Dmitry Baryshnikov [hidden email] [hidden email]
wrote:


Hi everybody,

Happy new year and lot of success in 2018!

I would like to discuss my pull request https://github.com/OSGeo/gdal/
pull/282

I created a thin wrapper around the json-c library which wide using in
GDAL.

This is C++ interface which hides C memory management and provides nice
API. The web or disk json documents reading chunk by chunk with progress
indication also added.

In future, the json-c can be easily switch to something other without
breaking the existing code.

The CPLJSONDocument/CPLJSONObject/CPLJSONArray usage examples can be
found in frmts/pds driver and c++ unit test in autotest/cpp/test_cpl.cpp.

Is this ready to merge into the trunk? Any objections?

Best regards,
    Dmitry


_______________________________________________
gdal-dev mailing [hidden email]




--
Sean Gillies

_______________________________________________
gdal-dev mailing [hidden email]







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

Re: Proposed updates to the GDAL feature style spec

Alan Thomas
In reply to this post by Alan Thomas
I have made the update in https://trac.osgeo.org/gdal/changeset/41230,
with corrections in https://trac.osgeo.org/gdal/changeset/41235.

You can view the refreshed spec at http://www.gdal.org/ogr_feature_style.html

Alan


--
Alan Thomas
Software Developer
ThinkSpatial
http://www.thinkspatial.com.au
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev