[pgrouting-dev] duplicate function names in core

pgRouting pgrouting-dev
1

Hello,

I am encountering problems placing the drivedist.c and
boost_drivedist.cpp into core (everything compiles, drivedist works
but astar searches dumps core. If I take out drivedist, astar works
like normal.), so I decided to poke around the source. I just found
out that there are so many duplicate functions (i.e.
finish(),text2char(),etc.) and struct names (edge_column,etc.) in each
of the C files.

Since C does not have any namespace functionality nor function
overloading, I don't have any idea which one will be called whenever
there is a library call. Scary since this will be difficult to debug.

Now that pgRouting is getting source contributions, I think this will
be a good time that we start discussing function naming conventions to
avoid duplicates as well as directory (core and extensions)
structures.

Regards,

Mario.

2

On 7/10/2012 5:23 AM, Mario Basa wrote:

Hello,

I am encountering problems placing the drivedist.c and
boost_drivedist.cpp into core (everything compiles, drivedist works
but astar searches dumps core. If I take out drivedist, astar works
like normal.), so I decided to poke around the source. I just found
out that there are so many duplicate functions (i.e.
finish(),text2char(),etc.) and struct names (edge_column,etc.) in each
of the C files.

Since C does not have any namespace functionality nor function
overloading, I don't have any idea which one will be called whenever
there is a library call. Scary since this will be difficult to debug.

Now that pgRouting is getting source contributions, I think this will
be a good time that we start discussing function naming conventions to
avoid duplicates as well as directory (core and extensions)
structures.

Mario,

UGH! Ideally one of two things should happen:

1. common functions with compatible signatures should be extracted into a common.c common.h or utils.c utils.h and get shared by all algorithms that need them.

2. local functions should all be declares "static" which means they are not visible or callable from references outside the file they are declared in.

So, if you have a C wrapper and it needs functions and no other files need to use these functions then they should be declared as static and placed in the C wrapper file.

If you have some function that multiple C files need to call or you C++ also needs to call, then it needs to go into a file like <namespace>_utils.c and all the functions in this file need to have a unique <namespace>_functionname() like signature.

I think this will minimize the changes we need to make to the existing code using "static" declarations for most things, and then moving the others into some namespace'd set of files.

How does this sound?

I like the idea of having multiple library .so files rather than one huge file because it keeps things separated but more so because it makes it easy to rebuild one library without effecting any of the others. I would like to know that if I change TRSP that there is little to zero change that I can impact the other algorithms.

Thank about a production environment where you have pgrouting running on lots of databases and in one database you want to add TRSP. By making it one big library you force a change on all the databases that did not need that change, but if TRSP is in a separate library then all the database that are not using it can be assured to be stable because they did not change. The worsted cases is that the new database with TRSP added to it might be unstable and if there is a collision of files like you describe the cases crashes, it only happens in the new database. Hmmm, maybe it doesn't work this way once the postgresql loads the library. But I don't remember having problems loading multiple libraries.

Thoughts?

-Steve

3

Hi Mario and Steve,

I like what Steve wrote. If there are reusable functions that can be used from more than one algorithm, then we should do so.

I would like to add that the separation of “core” and “extra” was only because of the CGAL and GAUL dependency. So someone, who didn’t need driving distance, shouldn’t have to struggle with compiling CGAL as well.

Now as we don’t need CGAL anymore since PostGIS 2.0, there is no need for “core” and “extra” anymore. GAUL is GPL as pgRouting and it didn’t change for many years. So we could even compile it together with pgRouting.

Instead of “core” and “extra” I would prefer a directory for every new pgRouting algorithm. It could then contain files like:

  • commons
  • sql
  • src
  • tests
  • docs_common.rst- <algorithm_name>
  • sql
  • <file_name>.sql- src
  • <file_name>.c
  • <file_name>.h
  • <file_name>.cpp- tests
  • README.txt
  • docs_.rst

For example we can then ask for each algorithm to contain tests and documentation.

Daniel

On Wed, Jul 11, 2012 at 2:38 AM, Stephen Woodbridge <woodbri@swoodbridge.com> wrote:

On 7/10/2012 5:23 AM, Mario Basa wrote:

Hello,

I am encountering problems placing the drivedist.c and
boost_drivedist.cpp into core (everything compiles, drivedist works
but astar searches dumps core. If I take out drivedist, astar works
like normal.), so I decided to poke around the source. I just found
out that there are so many duplicate functions (i.e.
finish(),text2char(),etc.) and struct names (edge_column,etc.) in each
of the C files.

Since C does not have any namespace functionality nor function
overloading, I don’t have any idea which one will be called whenever
there is a library call. Scary since this will be difficult to debug.

Now that pgRouting is getting source contributions, I think this will
be a good time that we start discussing function naming conventions to
avoid duplicates as well as directory (core and extensions)
structures.

Mario,

UGH! Ideally one of two things should happen:

  1. common functions with compatible signatures should be extracted into a common.c common.h or utils.c utils.h and get shared by all algorithms that need them.

  2. local functions should all be declares “static” which means they are not visible or callable from references outside the file they are declared in.

So, if you have a C wrapper and it needs functions and no other files need to use these functions then they should be declared as static and placed in the C wrapper file.

If you have some function that multiple C files need to call or you C++ also needs to call, then it needs to go into a file like _utils.c and all the functions in this file need to have a unique _functionname() like signature.

I think this will minimize the changes we need to make to the existing code using “static” declarations for most things, and then moving the others into some namespace’d set of files.

How does this sound?

I like the idea of having multiple library .so files rather than one huge file because it keeps things separated but more so because it makes it easy to rebuild one library without effecting any of the others. I would like to know that if I change TRSP that there is little to zero change that I can impact the other algorithms.

Thank about a production environment where you have pgrouting running on lots of databases and in one database you want to add TRSP. By making it one big library you force a change on all the databases that did not need that change, but if TRSP is in a separate library then all the database that are not using it can be assured to be stable because they did not change. The worsted cases is that the new database with TRSP added to it might be unstable and if there is a collision of files like you describe the cases crashes, it only happens in the new database. Hmmm, maybe it doesn’t work this way once the postgresql loads the library. But I don’t remember having problems loading multiple libraries.

Thoughts?

-Steve

pgrouting-dev mailing list
pgrouting-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/pgrouting-dev

–
Georepublic UG & Georepublic Japan
eMail: daniel.kastl@georepublic.de
Web: http://georepublic.de

4

Hello Steve,

With the problem I am encountering when I brought the driving_distance
into core, both boost_drivedist.cpp and astar_boost_wrapper.cpp have
graph_add_edge() function and Edge and Vertex structs.

The offending components here are the structs I think since they have
different values even if they have the same name in the c++ files. So,
whichever you compile first (astar or drivedist) in the CMAKE file
will work while the other will core dump. For fun, I made a different
library in the CMAKE for drivedist, and it still dumped core.

It was only after I renamed everything (Edge_dd,Vertex_dd, etc.) did
it work. Again, this is so difficult to debug, well time consuming
anyway. I am not so familiar with the recent gcc compiler, but I'll
try look if there is a compile time option to give out warnings for
similar function names.

I'll move the common functions into a pgr_utils.h file to take out
redundancy in core.

Mario.

On Wed, Jul 11, 2012 at 8:38 AM, Stephen Woodbridge
<woodbri@swoodbridge.com> wrote:

On 7/10/2012 5:23 AM, Mario Basa wrote:

Hello,

I am encountering problems placing the drivedist.c and
boost_drivedist.cpp into core (everything compiles, drivedist works
but astar searches dumps core. If I take out drivedist, astar works
like normal.), so I decided to poke around the source. I just found
out that there are so many duplicate functions (i.e.
finish(),text2char(),etc.) and struct names (edge_column,etc.) in each
of the C files.

Since C does not have any namespace functionality nor function
overloading, I don't have any idea which one will be called whenever
there is a library call. Scary since this will be difficult to debug.

Now that pgRouting is getting source contributions, I think this will
be a good time that we start discussing function naming conventions to
avoid duplicates as well as directory (core and extensions)
structures.

Mario,

UGH! Ideally one of two things should happen:

1. common functions with compatible signatures should be extracted into a
common.c common.h or utils.c utils.h and get shared by all algorithms that
need them.

2. local functions should all be declares "static" which means they are not
visible or callable from references outside the file they are declared in.

So, if you have a C wrapper and it needs functions and no other files need
to use these functions then they should be declared as static and placed in
the C wrapper file.

If you have some function that multiple C files need to call or you C++ also
needs to call, then it needs to go into a file like <namespace>_utils.c and
all the functions in this file need to have a unique
<namespace>_functionname() like signature.

I think this will minimize the changes we need to make to the existing code
using "static" declarations for most things, and then moving the others into
some namespace'd set of files.

How does this sound?

I like the idea of having multiple library .so files rather than one huge
file because it keeps things separated but more so because it makes it easy
to rebuild one library without effecting any of the others. I would like to
know that if I change TRSP that there is little to zero change that I can
impact the other algorithms.

Thank about a production environment where you have pgrouting running on
lots of databases and in one database you want to add TRSP. By making it one
big library you force a change on all the databases that did not need that
change, but if TRSP is in a separate library then all the database that are
not using it can be assured to be stable because they did not change. The
worsted cases is that the new database with TRSP added to it might be
unstable and if there is a collision of files like you describe the cases
crashes, it only happens in the new database. Hmmm, maybe it doesn't work
this way once the postgresql loads the library. But I don't remember having
problems loading multiple libraries.

Thoughts?

-Steve
_______________________________________________
pgrouting-dev mailing list
pgrouting-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/pgrouting-dev

5

Hi Mario,

I am not familiar with c/c++ specification,
but I had encountered the same problem before.
See my experimental “msvc” branch commit diff. https://github.com/sanak/pgrouting/commit/9ef417078eadad8554e1d7cd3ccd7592266b2912
(NOTICE: Don’t mind my temporary fix(symbol naming, .etc),
because my “msvc” branch is “only” experimental.)

And I agree that different symbol name using.

Regards,

2012/7/11 Mario Basa <mario.basa@gmail.com>

Hello Steve,

With the problem I am encountering when I brought the driving_distance
into core, both boost_drivedist.cpp and astar_boost_wrapper.cpp have
graph_add_edge() function and Edge and Vertex structs.

The offending components here are the structs I think since they have
different values even if they have the same name in the c++ files. So,
whichever you compile first (astar or drivedist) in the CMAKE file
will work while the other will core dump. For fun, I made a different
library in the CMAKE for drivedist, and it still dumped core.

It was only after I renamed everything (Edge_dd,Vertex_dd, etc.) did
it work. Again, this is so difficult to debug, well time consuming
anyway. I am not so familiar with the recent gcc compiler, but I’ll
try look if there is a compile time option to give out warnings for
similar function names.

I’ll move the common functions into a pgr_utils.h file to take out
redundancy in core.

Mario.

On Wed, Jul 11, 2012 at 8:38 AM, Stephen Woodbridge
<woodbri@swoodbridge.com> wrote:

On 7/10/2012 5:23 AM, Mario Basa wrote:

Hello,

I am encountering problems placing the drivedist.c and
boost_drivedist.cpp into core (everything compiles, drivedist works
but astar searches dumps core. If I take out drivedist, astar works
like normal.), so I decided to poke around the source. I just found
out that there are so many duplicate functions (i.e.
finish(),text2char(),etc.) and struct names (edge_column,etc.) in each
of the C files.

Since C does not have any namespace functionality nor function
overloading, I don’t have any idea which one will be called whenever
there is a library call. Scary since this will be difficult to debug.

Now that pgRouting is getting source contributions, I think this will
be a good time that we start discussing function naming conventions to
avoid duplicates as well as directory (core and extensions)
structures.

Mario,

UGH! Ideally one of two things should happen:

  1. common functions with compatible signatures should be extracted into a
    common.c common.h or utils.c utils.h and get shared by all algorithms that
    need them.

  2. local functions should all be declares “static” which means they are not
    visible or callable from references outside the file they are declared in.

So, if you have a C wrapper and it needs functions and no other files need
to use these functions then they should be declared as static and placed in
the C wrapper file.

If you have some function that multiple C files need to call or you C++ also
needs to call, then it needs to go into a file like _utils.c and
all the functions in this file need to have a unique
_functionname() like signature.

I think this will minimize the changes we need to make to the existing code
using “static” declarations for most things, and then moving the others into
some namespace’d set of files.

How does this sound?

I like the idea of having multiple library .so files rather than one huge
file because it keeps things separated but more so because it makes it easy
to rebuild one library without effecting any of the others. I would like to
know that if I change TRSP that there is little to zero change that I can
impact the other algorithms.

Thank about a production environment where you have pgrouting running on
lots of databases and in one database you want to add TRSP. By making it one
big library you force a change on all the databases that did not need that
change, but if TRSP is in a separate library then all the database that are
not using it can be assured to be stable because they did not change. The
worsted cases is that the new database with TRSP added to it might be
unstable and if there is a collision of files like you describe the cases
crashes, it only happens in the new database. Hmmm, maybe it doesn’t work
this way once the postgresql loads the library. But I don’t remember having
problems loading multiple libraries.

Thoughts?

-Steve

pgrouting-dev mailing list
pgrouting-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/pgrouting-dev

pgrouting-dev mailing list
pgrouting-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/pgrouting-dev

6

Hello Mario,

On 7/11/2012 3:25 AM, Mario Basa wrote:

Hello Steve,

With the problem I am encountering when I brought the driving_distance
into core, both boost_drivedist.cpp and astar_boost_wrapper.cpp have
graph_add_edge() function and Edge and Vertex structs.

The offending components here are the structs I think since they have
different values even if they have the same name in the c++ files. So,
whichever you compile first (astar or drivedist) in the CMAKE file
will work while the other will core dump. For fun, I made a different
library in the CMAKE for drivedist, and it still dumped core.

Yes, this could be a problem. I think that this is not a problem if each algorithm has its own directory because then the includes and code that uses them are all local to the directory. And this eliminates the cross contamination.

You still need to public interfaces to be consistent and not conflicting. For example, you would not want astar_sp() and drivedist() to return different structs using the same name, And you would not want to pass a struct or array of struct the is called the same thing but has different definitions BECAUSE these are public (ie: global) interfaces and redefinition in the global space will break things.

It was only after I renamed everything (Edge_dd,Vertex_dd, etc.) did
it work. Again, this is so difficult to debug, well time consuming
anyway. I am not so familiar with the recent gcc compiler, but I'll
try look if there is a compile time option to give out warnings for
similar function names.

Yes, this is very tricky to debug. Great work figuring this out.

I'll move the common functions into a pgr_utils.h file to take out
redundancy in core.

Excellent! Thanks for all your time and effort on this.

-Steve

Mario.

On Wed, Jul 11, 2012 at 8:38 AM, Stephen Woodbridge
<woodbri@swoodbridge.com> wrote:

On 7/10/2012 5:23 AM, Mario Basa wrote:

Hello,

I am encountering problems placing the drivedist.c and
boost_drivedist.cpp into core (everything compiles, drivedist works
but astar searches dumps core. If I take out drivedist, astar works
like normal.), so I decided to poke around the source. I just found
out that there are so many duplicate functions (i.e.
finish(),text2char(),etc.) and struct names (edge_column,etc.) in each
of the C files.

Since C does not have any namespace functionality nor function
overloading, I don't have any idea which one will be called whenever
there is a library call. Scary since this will be difficult to debug.

Now that pgRouting is getting source contributions, I think this will
be a good time that we start discussing function naming conventions to
avoid duplicates as well as directory (core and extensions)
structures.

Mario,

UGH! Ideally one of two things should happen:

1. common functions with compatible signatures should be extracted into a
common.c common.h or utils.c utils.h and get shared by all algorithms that
need them.

2. local functions should all be declares "static" which means they are not
visible or callable from references outside the file they are declared in.

So, if you have a C wrapper and it needs functions and no other files need
to use these functions then they should be declared as static and placed in
the C wrapper file.

If you have some function that multiple C files need to call or you C++ also
needs to call, then it needs to go into a file like <namespace>_utils.c and
all the functions in this file need to have a unique
<namespace>_functionname() like signature.

I think this will minimize the changes we need to make to the existing code
using "static" declarations for most things, and then moving the others into
some namespace'd set of files.

How does this sound?

I like the idea of having multiple library .so files rather than one huge
file because it keeps things separated but more so because it makes it easy
to rebuild one library without effecting any of the others. I would like to
know that if I change TRSP that there is little to zero change that I can
impact the other algorithms.

Thank about a production environment where you have pgrouting running on
lots of databases and in one database you want to add TRSP. By making it one
big library you force a change on all the databases that did not need that
change, but if TRSP is in a separate library then all the database that are
not using it can be assured to be stable because they did not change. The
worsted cases is that the new database with TRSP added to it might be
unstable and if there is a collision of files like you describe the cases
crashes, it only happens in the new database. Hmmm, maybe it doesn't work
this way once the postgresql loads the library. But I don't remember having
problems loading multiple libraries.

Thoughts?

-Steve
_______________________________________________
pgrouting-dev mailing list
pgrouting-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/pgrouting-dev

_______________________________________________
pgrouting-dev mailing list
pgrouting-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/pgrouting-dev