obs_api revision and improvements: new namespace etc. #530

busstoptaktik · 2017-06-28T10:27:44Z

The OBS API has been under development for about 12 months. The main intention is to provide access to functionality for dynamic datums and multistep transformations. A secondary intention is to improve name spacing and modularity of the code base.

This PR reflects a recent restructuring of the API architecture following input from discussions on the PROJ.4 mailing list. The material is still in a state of flux - among other things, the (sparse) documentation still refers to an older version of the API, so until it has been updated, it is not expected to make much sense.

rouault · 2017-06-28T10:37:21Z

src/pj_obs_api.c

-void pj_context_free (const PJ *P) {
+/* Create a new context - or provide a pointer to the default context */
+/* Prepared for more elaborate sscanf style setup syntax */
+PJ_CONTEXT *proj_context_create (char *setup, ...) {


should be const char*

rouault · 2017-06-28T10:38:01Z

src/pj_obs_api.c

+    va_list args;
+
+    /* The classic single threaded, backwards compatible way */
+    if (0==setup)


Hum, sounds a bit weird that a proj_context_create() function returns a default non-thread-safe context , but if this is documented...

rouault · 2017-06-28T10:39:23Z

src/pj_obs_api.c

+        return ctx;
+
+    /* Multi threaded with thread specific logging */
+    va_start (args, setup);


va_list are notoriously hard to use correctly. Just ask users and developers of libtiff TIFFGetField() / TIFFSetField()... Why not having setters on the PJ_CONTEXT* ?

I have never seen anyone having problems with sscanf/sprintf, and it is that mode of operation that was intended here.

We could have setters/getters, but if/as PJ_CONTEXT grows (se comment below) that would also mean some risk of API-sprawl.

Also, I would much prefer letting the context be a mostly immutable thing: If you need it to mutate, you should create a new one, and suffer the consequences. This is, however, not a firm pronciple of mine, just a matter of seeing the context as a stable scene on which the PJs do their live performance :-)

rouault · 2017-06-28T10:42:44Z

src/proj.h

+ *           After a year of experiments (and previous experience from the
+ *           trlib transformation library) it has become clear that the
+ *           context subsystem is unavoidable in a multi-threaded world.
+ *           Hence, instead of hiding it away, we move it into the limelight,


I must confess I'm not convinced at all this needs to be exposed, and this clutters the API, so we should really have strong reasons to expose it. Why wouldn't we have a hidden context per PJ* object. What are the use cases of sharing contexts among PJ* objects ?

I believe that even in the "old" API the concept of context could / should have stayed remained as an implementation detail of the projPJ structure

@rouault - The context provides the pj_errno that makes it possible to signal what went wrong in a non-succesful call to proj_create, where we (as in the classic pj_init) return 0 for "non-succesfull).

Yes, it would be nice to just set an errno on the returned object, but since we use a signalling null-pointer, to actually report what went wrong, we need an errno style variable, and we need it to be thread-local in the multithread case.

Hence my suggestion that we use errno, which is thread local on multithreaded systems, and which is intended to signal library call errors. But you argued against both this and explicitly thread local storage class pj_errno a few days ago - which is the reason I had to switch to explicit context handling.

You can't have it both ways - it's either signalling through errno (or a thread local storage class pj_errno) OR explicit contexts.

Why not a PJ * proj_create(PJ_CONTEXT *ctx, const char *definition, int *pnErrno) then ?

First, we could leave out the context in that case.

Second it does not really seem C-idiomatic (closest thing is strtod (const char* str, char** endptr), but to my C-eyes you set errno when you return a null-pointer, you set errno to signal what went wrong. Looking at the use of errno in pj_init, I actually also believe this was the original design, although setting errno to 0 inside a library should be a no-no: You can use it to signal an error, but should not stomp on the signal from a neighbouring function or library, if you do not have anything to tell.

Third, I believe we can have much potential use (experience from trlib) in explicit contexts, that e.g. can be used to handle grid catalogs on behalf of a number of PJs, or for debugging assistance through reference counting (yell if you close a context before all PJs on that context are destroyed) - and in general, the contexts give us another degree of freedom for working around less fortunate design decisions of former times.

But really, I would MUCH prefer to skip contexts totally and use (the inherently thread local) errno for messaging. However, in case that is not possible, we might as well get as much utility out of this as possible

errno can work of course, and I'd be fine if you decide this it the way to go. It is just I'm personnally always a bit unconfortable with it as you must take care of saving it before calling something else that might modify it behind your back.

Regarding contexts, if I sum up the situation, basically the choice is between "we don't need exposing explicit context today, so don't expose them" and "we don't need exposing explicit context today, but might at some later point, so expose them". I'll defer to your decision.

Regarding grid loading, we probably want grids to be a global resource protected by a global mutex, as done currently, so as to avoid memory usage to grow too much. Although I remember people complaining using proj in a multithreaded scenario, where one thread requiring grid loading would block other threads not needing grid. I might have fixed this in the past, or at least looked at that issue :-) Can't remember. So yes perhaps in this scenario having an explicit context option "load grids in this context, and not in the global context, so as to avoid being blocked by other threads" could be beneficial

rouault · 2017-06-28T10:43:38Z

src/proj.h

-void pj_free (PJ *P);
+PJ  *proj_create (PJ_CONTEXT *ctx, const char *definition);
+PJ  *proj_create_argv (PJ_CONTEXT *ctx, int argc, char **argv);
+PJ  *proj_create_pipeline (PJ_CONTEXT *ctx, const char *def_left, const char *def_right);


I thought pipelines where multi-stages transform. left and right seems to be a limitation to 2

I would prefer def_from and def_to instead. The idea is to have a function that let's a user create a pipeline that converts from one CRS to another CRS. Example:

PJ pipe = proj_create_pipeline(ctx, "+init:epsg:25832", "+init=epsg:25833");

which can then be used by the transformation functions. For now it will be implemented as a simple pipeline that uses WGS84 as a hub (early binding, and similar to the existing API), but eventually it can be expanded to use a more complicated setup based on the EPSG-database that makes the direct transformation (late binding).

Suggestions for a more descriptive name: proj_crs_to_crs, proj_create_crs_to_crs or proj_crs_to_crs_pipeline

Pipelines are multi stage. This is a pipelined equivalent to the pj_transform case of utilizing a pivot datum, but with the added potential for searching for a real solution elsewhere (e.g. in the epsg file). Ordinary pipelines are instantiated through ordinary pj_create calls

rouault · 2017-06-29T10:13:02Z

I have never seen anyone having problems with sscanf/sprintf,

Huh, this doesn't match my experience. Before "recent" gcc versions, that now warn if the format string is inconsistent with the parameter list, this was a common problem to get it wrong and only realize at runtime. va_list use works, but requires really special care as the compiler cannot check anything for you. For example, you could pass a function that wouldn't have the expected prototype.

This is, however, not a firm pronciple of mine, just a matter of seeing the context as a stable scene on which the PJs do their live performance :-)

Note: a context should only be used by one thread at a time, so you normally shouldn't modify it whereas it is used by another proj_ function. Not exposing setters indeed avoids that potential issue. But you shouldn't also use different PJ* objects creating with the same context from different threads.

busstoptaktik · 2017-06-29T11:56:57Z

@rouault - That's exactly my point: Contexts are thread specific, errno is thread local. If we use contexts for nothing but pj_errno, we might as well use errno and get rid of contexts altogether.

But now that we seem to be stuck with contexts, we might as well prepare to get additional utility from them.

… fwd/invcoord to PJ

…e more with added parens

…, mostly in test code, to reflect API changes

rouault · 2017-07-04T14:55:21Z

src/pj_obs_api.c

+    /* The slightly convolved incremental indexing is used due */
+    /* to the stride, which may be any size supported by the platform */
+    for (i = 0; i < nmin; i++) {
+        coord.xyzt.x = *x;


This will only work on Intel and other architectures that accept unaligned access. For something more robust accross architectures, use memcpy(&(coord.xyzt.x, x, sizeof(double))

Or should we mention in the doc that despite the stride being in bytes, users should make sure that &x[stride*some_value] is propertly aligned ? In which case it would be safer to revert to the stride being specified in number of doubles

The alignment will be OK because sizeof(whatever) includes the necessary alignment slack on platforms that need it - otherwise foo *p = calloc (300, sizeof(foo)); would not work: p[0].bar is situated sizeof(foo) bytes from p[1].bar, the same is the case for p[0].baz and p[1].baz.

What we cannot assume is that sizeof(foo)==sum_of_the_size_of_its_elements, since there may be slack both between the struct elements, and at the end of the struct (but not at the beginning: The first element of a struct is required to live at the same address as the struct itself).

This is the reason for my comment about "any size supported by the platform", i.e. any size you can get from the sizeof-operator.

OK OK, so that confirms we require that x, y, z, t array addresses and the stride provided are properly setup for dereferencing double values. And that we don't pretend to support unaligned accesses, which is reasonable. Could be worth mentionning in the function doc so that people don't have false expectations ( someone could devise a struct { double* y; double* x; char c[1]; } attribute ((packed)) and expect proj to properly work on that. I admit this is convoluted )

Yes, certainly convoluted, but strange things happen in the wild, so I will try to put emphasis on this in the docs....

rouault · 2017-07-04T14:55:53Z

src/pj_obs_api.c

+
+        /* in all full length cases, we overwrite the input with the output */
+        if (nx > 1)  {
+            *x = coord.xyzt.x;


Same here: memcpy(x, &(coord.xyzt.x), sizeof(double)) (and for the below *y, *z, *t)

... and same reply :-)

rouault · 2017-07-04T14:56:46Z

src/pj_obs_api.c

+    /* ... or would we rather not? Then what about the nmin==1 case? */
+    /* perhaps signalling the non-array case by setting stride to 0? */
+    if (nx==1)
+        *x = coord.xyzt.x;


memcpy(...)

rouault · 2017-07-04T14:58:48Z

src/pj_obs_api.c

+        3. of length one, i.e. a constant, which will be treated as a fully
+           populated array of that constant value
+
+    The stride represents the step length, in bytes, between consecutive elements


If we want to be super general, perhaps the stride should be a stridex, stridey, stridez, stridet ? Sometimes for performance reasons you pack together x,y in the same structure, and have a dedicated array for z.

Sounds like a good idea, although it will stretch an already long parameter list a bit more. I'm sure you have seen many more real world use cases than I have, so I will look into it.

Would this be useful for use cases in GDAL, or is it more a general comment? For example, I agree that in coming years, we probably will see quite a few cases of "grafting elevation support onto legacy code", by adding a separate elevation array to an established 2D code base :-)

rouault · 2017-07-04T15:01:32Z

src/pj_obs_api.c

+    This is similar to the inner workings of the pj_transform function, but the
+    idea of the stride has been generalized to work for any size of basic unit,
+    not just a number of doubles.
+


Document the meaning of the return value ?

Sure - there is a lot of proper documentation writing ahead - right now I'm just trying to get the most important things in as comments. I will add the thing about returning the number of coordinates actually transformed.

rouault

Pleased to see this array based transformer

rouault · 2017-07-04T15:10:16Z

src/projects.h

+        will expose it to macro expansion, to the tune of much confusion and agony.
+
+    **************************************************************************************/
+    int last_errno;


I'm not really enthousiastic about this new error mechanism being added... Furthermore, apparently, it isn't used in the code. And shouldn't that declaration be extern int last_errono to avoid it being redefined in all compilation units that include projects.h ?

No, it should not be extern int: It is a part of the PJ object, so externing it would be a syntax error.

The PJ local last_error is used in proj_errno_set(), and since I'm trying to introduce some slightly more sane, but backwards compatible and not very intrusive, error reporting mechanism for the OBS API. I think adding an int (i.e. typically 4 or 8 bytes) to the already rather large PJ object is a small price to pay.

I see now that proj_errno lacks a check for the PJ local last_error, which I will add in a moment - the point is that it should return the local if nonzero, otherwise go on and search for the context specific, and (perhaps) finally the default context specific, because not all corners of the code can be supposed to support the PJ local reporting at once.

Note that the PJ local errno bubbles up to the context level (se comment in code), for compatibility with proj_api.h and projects.h based code.

Ah sorry I didn't see with the diff format that this was a new field in a structure... So disregard this part of my comment.

rouault · 2017-07-04T15:11:17Z

src/proj.h

@@ -328,6 +340,17 @@ extern const PJ      *proj_shutdown;
 #endif


+/* Set or read error level */
+int  proj_errno (PJ *P);
+void proj_errno_set (PJ *P, int err);


I don''t see the point in having proj_errno_set() and proj_context_errno_set() in the public API. They should be in a proj_private.h

They are quite useful for reporting from application specific functions, that take PJ arguments: In the main program one just need to do one kind of error checking, not having to care whether an error was reported from the code in function_doing_my_proj_stuff(PJ *P, ...) or in the API code called by that function.

pj_ctx_set_errno() is also part of proj_api.h - I assume for the same reason.

"""They are quite useful for reporting from application specific functions, that take PJ arguments.""" Hum, given that proj errno numbers are an internal detail, I don't see how user applications could know which code to use.

I'm just fighting to keep proj.h at the very strict minimum to reduce the API surface.

"pj_ctx_set_errno() is also part of proj_api.h - I assume for the same reason" : I'm pretty sure this wasn't really intended to be used by applications, but just because there is no proj internal include file. That should be there.

Yes, the proj errno numbers (which are negative) are quite specific (and actually much too specific in my opinion), but pj_strerrno() supports the generic POSIX positive errno values as well. Especially I tend to use EINVAL (invalid argument) symbol for quite a lot of generic reporting. While not strictly C89 (I think only EDOM and ERANGE are C89 required), I am yet to see a platform where EINVAL is not defined.

Otherwise, I much agree that fighting for slim APIs is the good fight: Much of the chaos of PROJ.4 is the result of API sprawl. Obviously creeping featurism and clunky design is easier to realise in hindsight, than to prevent in foresight.

And care, good taste, real world experience, and a sharp razor are the right tools to ensure that an API stays fresh.

I just think my real world experience differ from yours wrt. error reporting: I have always found it very useful to be able to set an error level on a state object from user/application level code, and to try to make sure that system level code turned into no-ops when handed an error flagged state object.

So I would much prefer to keep pj_set_errno() in the API. I would probably be more eager to move pj_context_errno_set() to a proj_plumbing.h (or whatever) internal system tools file.

Regarding the lack of a proj internal include file: Wasn't the idea of introducing proj_api.hto demote projects.h to "internal include" status?

I much agree that fighting for slim APIs is the good fight

Pleased to see we are aligned on this objective

I just think my real world experience differ from yours wrt. error reporting: I have always found it very useful to be able to set an error level on a state object from user/application

OK, if this is on purpose, let's go for it

Regarding the lack of a proj internal include file: Wasn't the idea of introducing proj_api.hto demote projects.h to "internal include" status?

Perhaps, but the issue is that projects.h was already a installed header (since it is include_HEADERS in https://github.com/OSGeo/proj.4/blob/master/src/Makefile.am#L9), so there's always a temptation from API users to use things even if marked as internal details. I really thing using a proj_internal.h that is not installed is the clean way of keeping internal details hidden reliably

Perhaps, but the issue is that projects.h was already a installed header (since it is include_HEADERS in

It says in the top of projects.h that it is to be considered private. It was excluded from include_HEADERS in version 4.8, but was re-introduced with 4.9 because there was some complaints about missing functionality in proj_api.h. I think it is fair to remove it from the public once again and rename it proj_internal.h. Obviously not in the comming release, but the next as I suggested on the mailing list.

OK. Looks like a good plan. Perhaps there should be a ticket to checklist those future actions so that we don't forget...

Yeah, I have been thinking about setting up a project here on GitHub for the coming release. I'll try to play around with that today I think.

rouault · 2017-07-04T17:19:31Z

Would this be useful for use cases in GDAL, or is it more a general comment?

I don't think that GDAL would need that currently, but this is more a provision. Actually I had rather in mind a MapServer related situation where we might potentially have in the future x,y and z separated.

…uault

rouault · 2017-07-04T20:36:25Z

src/proj.h

@@ -287,14 +287,13 @@ PJ_OBS   proj_trans_obs   (PJ *P, enum proj_direction direction, PJ_OBS obs);
 PJ_COORD proj_trans_coord (PJ *P, enum proj_direction direction, PJ_COORD coord);


-int proj_transform (
+size_t proj_transform (


I suggested to get rid of most, if not all, include files at top of proj.h, but the use of size_t requires #include <stddef.h>.

Thanks - both for the original suggestion, and for this reminder about it. There really had accumulated a lot of cruft at the top. It is now much reduced

Great. What could help is to have a simple proj_test_include_proj_h.c that just does #include "proj.h" . I've done something similar recently in GDAL to check that public headers are really well self contained.

…tions from Even Rouault

busstoptaktik · 2017-07-04T21:49:58Z

I tried something like: #include <proj.h> #include <stdio.h> int main (void) { puts ("proj.h included successfully"); } Which worked as suspected. Is this what you suggested? 2017-07-04 23:10 GMT+02:00 Even Rouault <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In src/proj.h <#530 (comment)>: > @@ -287,14 +287,13 @@ PJ_OBS proj_trans_obs (PJ *P, enum proj_direction direction, PJ_OBS obs); PJ_COORD proj_trans_coord (PJ *P, enum proj_direction direction, PJ_COORD coord); -int proj_transform ( +size_t proj_transform ( Great. What could help is to have a simple proj_test_include_proj_h.c that just does #include "proj.h" . I've done something similar recently in GDAL to check that public headers are really well self contained. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#530 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFWTrUSJFhN8S7ZebSfCb53z5ENpuM3Vks5sKqoqgaJpZM4OHxvE> .

rouault · 2017-07-04T21:54:13Z

I tried something like: #include <proj.h> #include <stdio.h> int main (void) { puts ("proj.h included successfully"); } Which worked as suspected. Is this what you suggested?

Yes, You can even reduce to return 0; and not output anything ;-)

busstoptaktik · 2017-07-04T22:06:09Z

OK - works flawlessly for all of projects.h, proj_api.h and proj.h (compiling with -W -Wall -Wextra -pedantic), as they stand now. I do, however, suspect that a few #includes can be removed from both projects.h and proj_api.h :-) 2017-07-04 23:54 GMT+02:00 Even Rouault <notifications@github.com>:

…

I tried something like: #include <proj.h> #include <stdio.h> int main (void) { puts ("proj.h included successfully"); } Which worked as suspected. Is this what you suggested? Yes, You can even reduce to return 0; and not output anything ;-) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#530 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFWTrZ4M02e7YJSCjUajLw884dt4tVe7ks5sKrSFgaJpZM4OHxvE> .

…ssue, as another check has locked the path, but at least this should calm down MSVC

…uff from pj_obs_api.c to pj_internal.c

kbevers · 2017-07-05T14:37:18Z

src/lib_proj.cmake

@@ -228,6 +228,7 @@ set(HEADERS_LIBPROJ
        proj_api.h
        proj.h
        geodesic.h
+        proj_internal.h


Will this not make proj_internal.h public?

busstoptaktik · 2017-07-05T15:41:21Z

It will, and has been removed again. It was introduced intermediately while debugging some intractable build system mysteries :-) 2017-07-05 16:37 GMT+02:00 Kristian Evers <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In src/lib_proj.cmake <#530 (comment)>: > @@ -228,6 +228,7 @@ set(HEADERS_LIBPROJ proj_api.h proj.h geodesic.h + proj_internal.h Will this not make proj_internal.h public? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#530 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFWTrWb13lQhBcvpQZ9OKTw-gpP8fdTiks5sK5-fgaJpZM4OHxvE> .

kbevers · 2017-07-06T08:21:38Z

src/pj_obs_api.c

-void pj_context_inherit (PJ *mother, PJ *daughter) {
-    pj_set_ctx (daughter, pj_get_ctx (mother));
+/* Create a new context - or provide a pointer to the default context */
+PJ_CONTEXT *proj_context_create (int multithreaded) {


Wouldn't it be simpler to leave out the multithreaded argument? If you want the default context you call proj_create(0, "...") and if you want a non-default context you create it be calling proj_context_create(). In case you want to prepare your application for multithreaded use, but is still running it singlethreaded a non-default context can be used with out problem:

PJ_CONTEXT * ctx proj_context_create(); PJ * P proj_create(ctx, "+proj=..."); ... proj_context_destroy(ctx); proj_destroy(P);

To me that is a little bit cleaner and simpler.

I agree - and it comes with the extra feature of being easier to document and demonstrate :-)

Will change that, and update the demo samples accordingly.

kbevers · 2017-07-06T08:44:29Z

Right now there are two prefixes in play: proj_ and PJ_. How should they be interpreted by users? At first glance it looks like proj_ is reserved for functions and PJ_ for types, but then you run into PJ_TORAD() which is somewhat a function, but not really. Is the namespace completely corehent here?

busstoptaktik · 2017-07-06T09:04:01Z

Coherency is for tailors :-)

Switching to a function implementation would make it natural to move it to the proj_ namespace, and furthermore probably would allow us to remove the #include <math.h> with its make-Microsoft-play-nice related incantations from proj.h.

Seems worthwhile - will implement during the afternoon.

@kbevers

…an Evers @kbevers

rouault · 2017-07-06T12:05:43Z

src/proj.h

+int  proj_errno_reset (PJ *P);
+void proj_errno_restore (PJ *P, int err);
+
+char *proj_get_definition (PJ *P);


To avoid potential cross heap issues on Windows (if proj is compiled with MSVC X and the application integrating it with MSVC Y), we should provide a proj_free() (if that name is still avaialble) that does the free inside the lib.
( with the "old" proj_api.h, this was the pj_get_def() / pj_dalloc() tuple)

Thanks for spotting this! It could result in some really nasty debugging sessions. In a recent commit, I have added the function proj_buffer_free, as (mostly) a synonym for pj_dealloc.

@rouault @kbevers - I believe this is ready for merging. Do you agree?

If the tests pass, definitely ;-) I'll probably review once again all changes once they are committed.
Remark: It doesn't look like proj_get_definition and proj_buffer_free are added to proj.def. The maintenance of this file is really painful. Wondering if we shouldn't have a PROJ_EXPORT macro that would be used in the .h directly and would expand to __declspec(dllexport) on Windows, but that's unrelated with this PR

Sure, go ahead when all tests pass :)

…tential cross heap issues on Windows. Thx to Even Rouault for spotting this

busstoptaktik · 2017-07-06T13:06:10Z

Oops - and I forgot a few more as well. Correcting, committing and pushing in a few minutes :-) 2017-07-06 14:47 GMT+02:00 Even Rouault <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In src/proj.h <#530 (comment)>: > void proj_errno_set (PJ *P, int err); -/* reduce some mental impedance in the canonical reset/restore use case */ -#define proj_errno_restore(P, err) proj_errno_set ((P), (err)) +int proj_errno_reset (PJ *P); +void proj_errno_restore (PJ *P, int err); + +char *proj_get_definition (PJ *P); If the tests pass, definitely ;-) I'll probably review once again all changes once they are committed. Remark: It doesn't look like proj_get_definition and proj_buffer_free are added to proj.def. The maintenance of this file is really painful. Wondering if we shouldn't have a PROJ_EXPORT macro that would be used in the .h directly and would expand to __declspec(dllexport) on Windows, but that's unrelated with this PR — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#530 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFWTrSee2T8szBe8pxmOFxrgatIHdKEYks5sLNdhgaJpZM4OHxvE> .

… removed some not-yet-implemented material from proj.h

rouault · 2017-07-06T13:23:27Z

src/proj.h

@@ -341,6 +341,9 @@ void proj_errno_restore (PJ *P, int err);

 char *proj_get_definition (PJ *P);

+/* deallocate anything obtained from PROJ.4 */


anything... except a PJ* that should be destroyed with proj_destroy()

Yes - will make that clear when getting around to writing the formal documentation, and update the comment for clarity

... actually - for clarity and API symmetry with the PJ_CONTEXT and PJ cases, I changed the two names to proj_definition_create() and proj_definition_destroy(). It may become necessary with API level access to a generic low level deallocator, but right now its only use is to get rid of material produced by proj_definition_create

Hum proj_definition_create() is a bit confusing for something that is essentially a getter

Down at the library level it is not exactly a getter (that could just be the case of returning P->params) ... "create" is much in accordance with the actual construction work done by pj_get_def: It actually walks the P->param list, element by element expanding the return string.

Now, the user does not need to know this, but for reasons of communication, I believe it is a good thing to have the reminder that "what you get by a constructor, you get rid of by a destructor"

So, @rouault and @kbevers : What's to be done here? I seriously dislike the first solution: While proj_get_definition() / proj_buffer_free() is a fairly mindless 1:1 translation from the original pj_get_def() / pj_dalloc() implementation, it is totally orthogonal to current semantics of the proj_xxx API.

I can think of the following solutions:

As-is: A create/destroy pair, that clearly signals that what's created must be destroyed - when you start a paranthesis, you must also provide a corresponding end paranthesis at the proper syntactic and semantic point. I evidently much prefer this solution.

As-was: The original proj_get_definition() / proj_buffer_free() pair, where the constructor/destructor pair is referentially asymmetric, where the constructor is specific, while the destructor is generic, and where, to add insult to injury, the naming is orthogonal to everything else in the proj_xxx API. I very much dislike this solution.

As-maybe-to-be: A not-really-getter-but-not-either-referentially-a-getter: We could introduce a proj_definition() function, which would be referentially in alignment with the proj_error() function, but would need a low level deallocator, which would be referentially assymetric to the constructor/pseudogetter, but which, on the positive side, could be play the role of a C++ default-destructor. In acknowledgement of the humble low-level building bricks of PROJ.4, I would prefer to call this function proj_dealloc(). I could certainly live reasonably happily with this solution, although I would very much prefer to make it more clear when something is constructed for the API user, and hence also should be destroyed by the API user.

There is probably at least half-a-dozen additional potential solutions, that I cannot think of right now - but have a go: Suggestions? Opinions?

…e/proj_definition_destroy, for symmetry and clarity

busstoptaktik · 2017-07-06T21:00:06Z

Repeating this comment at a more reasonable place in the thread:

So, @rouault and @kbevers : What's to be done here? I seriously dislike the first solution: While proj_get_definition() / proj_buffer_free() is a fairly mindless 1:1 translation from the original pj_get_def() / pj_dalloc() implementation, it is totally orthogonal to current semantics of the proj_xxx API.

I can think of the following solutions:

As-is: A create/destroy pair, that clearly signals that what's created must be destroyed - when you start a paranthesis, you must also provide a corresponding end paranthesis at the proper syntactic and semantic point. I evidently much prefer this solution.
As-was: The original proj_get_definition() / proj_buffer_free() pair, where the constructor/destructor pair is referentially asymmetric, where the constructor is specific, while the destructor is generic, and where, to add insult to injury, the naming is orthogonal to everything else in the proj_xxx API. I very much dislike this solution.
As-maybe-to-be: A not-really-getter-but-not-either-referentially-a-getter: We could introduce a proj_definition() function, which would be referentially in alignment with the proj_error() function, but would need a low level deallocator, which would be referentially assymetric to the constructor/pseudogetter, but which, on the positive side, could be play the role of a C++ default-destructor. In acknowledgement of the humble low-level building bricks of PROJ.4, I would prefer to call this function proj_dealloc(). I could certainly live reasonably happily with this solution, although I would very much prefer to make it more clear when something is constructed for the API user, and hence also should be destroyed by the API user.

There is probably at least half-a-dozen additional potential solutions, that I cannot think of right now - but have a go: Suggestions? Opinions?

rouault · 2017-07-06T21:09:33Z

Suggestions?

Another possibility could be a const char* proj_get_definition(PJ*) with no corresponding free, since the PJ* object would retain the string in the structure and deallocate it when PJ* is freed. The downside of it is that people shouldn't use this const char* after having destroyed PJ*, but that's something reasonable for users of the API

I don't want to be punctilious, so basically I let it to your judgement for the best solution. This is just the naming create_definition() which is a bit weird at first since the PJ* object itself was created from a definition that is close to the one that will be returned.

busstoptaktik · 2017-07-06T22:10:05Z

This is just the naming create_definition() which is a bit weird at first since the PJ* object itself was created from a definition that is close to the one that will be returned.

I see your point - in many ways it is parallel to my introduction yesterday of proj_errno_restore() as a synonym for proj_errno_set(), because the latter would, in the most prolific use case, be linguistically confusing.

With this in mind, I could imagine a somewhat parallel, but more general, case where we introduce the suffix pair _retrieve() / _dispose() for cases where _create() / _ destroy() would be semantically and syntactically sufficient, but linguistically confusing, i.e. for cases that really are constructor/destructor pairs, but where users wouldn't believe so unless we dragged them violently backwards through the inner machinery of PROJ.4

We might even try to make do with a single proj_default_dispose() (or simply proj_dispose() - so @rouault and @kbevers: what do you think about the pairs

proj_definition_retrieve() / proj_definition_dispose()
proj_definition_retrieve() / proj_default_dispose()
proj_definition_retrieve() / proj_dispose()

The two latter will require some future care, to make sure that we never introduce retrievers for anything that cannot be disposed of by a plain (but masked) call to free()

busstoptaktik · 2017-07-07T05:09:13Z

(extended/continued)
@rouault and @kbevers: what do you think about the pairs

proj_definition_retrieve() / proj_definition_dispose()
proj_definition_retrieve() / proj_default_dispose()
proj_definition_retrieve() / proj_dispose()
proj_definition_retrieve() / proj_release()
proj_definition_retrieve() / proj_free()

With an overall semantic structure saying that

What you _create(), you must _destroy(), and
what you _retrieve(), you must _release() (or _dispose()or _free()), but
what you _get(), you may (_)keep.

Hence a structure, where create/delete governs the life cycle of state objects, retrieve/release governs the life cycle of material derived from state objects, that requires allocation on the heap, while get-style functions return material on the stack, that is either a reflection of internal state (fields), or derived from these.

I did a test implementation using proj_definition_retrieve() / proj_free(), and it looks pretty neat. I probably would have preferred proj_release(), had it not been for the use of release functions in pj_mutex.c.

Obviously, I resisted the temptation to define a proj_keep() null macro for symmetry in the _get() case :-)

kbevers · 2017-07-07T07:43:43Z

proj_definition_retrieve() / proj_release()

I like this one.

With an overall semantic structure saying that

What you _create(), you must _destroy(), and

what you _retrieve(), you must _release() (or _dispose()or _free()), but

what you get(), you may ()keep.

I think this structure makes sense. Are you suggesting a generic proj_release() (or whatever) or do you want to keep the symmetry and have proj_retrieve_definition() paired with proj_release_definition() ?

busstoptaktik · 2017-07-07T13:31:51Z

I think this structure makes sense. Are you suggesting a generic proj_release()

Yes - stuff that can be free'd by a default destructor can be retrieved (other stuff must be constructed, and a type specific destructor must be called).

And since we may call the default constructor whatever we want, I will follow your vote and rename proj_free to proj_release, wait for tests to pass, and then squash+merge.

obs_api revision and improvements: new namespace etc.

b54d3c7

busstoptaktik self-assigned this Jun 28, 2017

rouault reviewed Jun 28, 2017

View reviewed changes

busstoptaktik added 6 commits June 29, 2017 15:19

Minor corrections: use unused functions, add missing prototype, added…

996cb68

… fwd/invcoord to PJ

Minor correction: MSVC barfs on va_arg type specification. Trying onc…

d9d3e5c

…e more with added parens

Reverting paren correction, which appears to be a non-solution

31b7f1e

Significant improvements to the OBS_API, plus a number of corrections…

672a89e

…, mostly in test code, to reflect API changes

Added two missing prototypes

5042593

Adding the proj_transform function and some related checks

5bad0d2

rouault reviewed Jul 4, 2017

View reviewed changes

Improvements to proj_transform etc. based on suggestions from Even Ro…

abd3e5f

…uault

rouault reviewed Jul 4, 2017

View reviewed changes

Reducing the libc include footprint of proj.h - again based on sugges…

75b0afe

…tions from Even Rouault

busstoptaktik added 4 commits July 5, 2017 00:07

A few minor corrections

6fcce34

Eliminate a MSVC warning about non-initialized usage. Not an actual i…

2e6fece

…ssue, as another check has locked the path, but at least this should calm down MSVC

Improved support for the errno reset/restore paradigm

f188c08

Introduced the internal header proj_internal.h; Moved most non-API st…

ca831c6

…uff from pj_obs_api.c to pj_internal.c

busstoptaktik added 2 commits July 5, 2017 15:44

Correcting a few additional blunders in PJ_pipeline.c

6c23a03

Changed angle-brackets to quotation marks in includes of proj_internal.h

8837882

kbevers reviewed Jul 5, 2017

View reviewed changes

Some minor build system repairs

26d7ed7

kbevers reviewed Jul 6, 2017

View reviewed changes

busstoptaktik added 4 commits July 6, 2017 11:59

Some PJ_CONTEXT usage simplifications following suggestions by Kristi…

9529e5a

…an Evers @kbevers

Added version numbering to proj.h - Fixes #529

524f838

remove proj_errno_restore macro, provide function implementation

c762d9a

Add proj_get_definition. Fixes #538

b400b03

rouault reviewed Jul 6, 2017

View reviewed changes

busstoptaktik added 2 commits July 6, 2017 14:39

Added library specific deallocator proj_buffer_free, eliminating a po…

d623b72

…tential cross heap issues on Windows. Thx to Even Rouault for spotting this

Got rid of a meaningless cast in proj_buffer_free

4813217

Added some missing functions to proj.def (again spotted by @rouault);…

95cc428

… removed some not-yet-implemented material from proj.h

rouault reviewed Jul 6, 2017

View reviewed changes

Renamed proj_get_definition/proj_buffer_free to proj_definition_creat…

91ca80e

…e/proj_definition_destroy, for symmetry and clarity

Renaming the definition handlers to proj_definition_retrieve / proj_free

8d06f64

Renaming proj_free to proj_release

7775ebc

busstoptaktik merged commit e09e24e into OSGeo:master Jul 7, 2017

busstoptaktik deleted the obs_api_maintenance branch July 7, 2017 15:15

busstoptaktik mentioned this pull request Jan 3, 2022

CoordinateTuple should be a trait, rather than a type busstoptaktik/geodesy#25

Closed

		@@ -341,6 +341,9 @@ void proj_errno_restore (PJ *P, int err);

		char proj_get_definition (PJ P);

		/* deallocate anything obtained from PROJ.4 */

obs_api revision and improvements: new namespace etc. #530

obs_api revision and improvements: new namespace etc. #530

Conversation

busstoptaktik commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busstoptaktik Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbevers Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouault commented Jun 29, 2017

busstoptaktik commented Jun 29, 2017

rouault Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

busstoptaktik Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouault Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouault Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busstoptaktik Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbevers Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouault commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busstoptaktik commented Jul 4, 2017 via email

rouault commented Jul 4, 2017

busstoptaktik commented Jul 4, 2017 via email

Choose a reason for hiding this comment

busstoptaktik commented Jul 5, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbevers commented Jul 6, 2017

busstoptaktik commented Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busstoptaktik Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busstoptaktik commented Jul 6, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busstoptaktik commented Jul 6, 2017

rouault commented Jul 6, 2017

busstoptaktik commented Jul 6, 2017

busstoptaktik Jun 29, 2017 •

edited

Loading

kbevers Jun 28, 2017 •

edited

Loading

rouault Jul 4, 2017 •

edited

Loading

busstoptaktik Jul 4, 2017 •

edited

Loading

rouault Jul 4, 2017 •

edited

Loading

rouault Jul 4, 2017 •

edited

Loading

busstoptaktik Jul 4, 2017 •

edited

Loading

kbevers Jul 4, 2017 •

edited

Loading

busstoptaktik commented Jul 6, 2017 •

edited

Loading

busstoptaktik Jul 6, 2017 •

edited

Loading