Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

To C++ pg data get, fetch and check #2504

Merged
merged 49 commits into from May 4, 2023
Merged

Conversation

cvvergara
Copy link
Member

@cvvergara cvvergara commented Apr 6, 2023

During the last 10 years, an effort to standarize code in pgRouting has been done.
With the new trsp functions, finnaly, the code is standarized.
(It still have the code of the old signature of pgr_trsp.)

By standarize, I mean, the C/C++ code is similar from one function to another.
As a first step:
Comparing coordinates_input with delauny_input (et all)
They all look similar.

This are fetchers:

In general, this is code common to both functions

Except for the code

  • The function name
  • for coordinates: from to
  • for delauny: from to

The function name and the different code is what is now part of the pgdata_getters.

  • They are compiled as C++ code
  • They are used on C code, on the static void process function in all the *.c files connecting to postgres.
  • Therefore they are linked as C code

The similar code is what is now a template header. include/cpp_common/get_data.hpp added in the first commit of this
PR.

  • Only C++ code can use it.

The fetchers:

  • are called by the template
  • therefore only used by C++ code

The postgresql_connection:

  • They are compiled as C code
  • Used on C code, on the static void process function in all the *.c files connecting to postgres.
  • Used on the new C++ template file

The get_check_data

  • Only C++ code can use it.
  • Uses external postgres C code

why this change is good

Adding a new inner query implies:
Before this change:

  • add a new file that has the fetcher and getter (maybe by copying from another file)
  • Adjusting the code, being extra careful with the types, pointers, memory allocation

After this change

  • add a new getter in the pgdata_getters file
    • No worry of types, pointers, memory allocation etc.
  • add a new fetcher in the pgdata_fetcher file
    • No worry of types, pointers, memory allocation etc.

drawback

Support for compiling with MSVC has to end.

D:/a/pgrouting/pgrouting/pgsql/pgsql/include/server\port/atomics/generic-msvc.h(83,12): message : while trying to match the argument list '(volatile uint64 *, uint64, uint64)' [D:\a\pgrouting\pgrouting\build\src
\cpp_common\cpp_common.vcxproj]
D:/a/pgrouting/pgrouting/pgsql/pgsql/include/server\port/atomics/generic-msvc.h(97,9): error C2664: 'LONG64 _InterlockedExchangeAdd64(volatile LONG64 *,LONG64)': cannot convert argument 1 from 'volatile uint64 *
' to 'volatile LONG64 *' [D:\a\pgrouting\pgrouting\build\src\cpp_common\cpp_common.vcxproj]
D:/a/pgrouting/pgrouting/pgsql/pgsql/include/server\port/atomics/generic-msvc.h(97,35): message : Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or parenthesized function-styl
e cast [D:\a\pgrouting\pgrouting\build\src\cpp_common\cpp_common.vcxproj]

Where is that error happening?

Is pgRouting the first one to encounter this problem?
No: extension plv8 also has the problem

How is it solved:
By patching postgres: https://plv8.github.io/#patching-postgres

I did had a conversation with @JerrySievert from plv8, on postgres slack channel on March 27, 2023.
I quote:

yeah, that's part of the problem of dealing with c++ and postgres, but as far as i've been able to tell over the
years, this is the only postgres-specific msvc++ build issue. of course you also have to worry about msvc++ not
actually implementing the standards it claims to support.
and that is why plv8 may or may not compile on windows currently.

do pgRouting developers need MSVC?

  • No. Many reasons, take your pick:
    • We develop on Open Source operative system.
    • it can be compiled with mingw64 (msys2) See here

Statistics

Less code to maintain. 👍

Currently (in develop)

cloc 0d4985b19b
    1715 text files.
    1708 unique files.
     626 files ignored.

github.com/AlDanial/cloc v 1.90  T=0.55 s (1983.3 files/s, 426992.8 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
PO File                           8          21019            598          62727
SQL                             319           4822           6774          40403
C++                             106           3323           3307          12770
C/C++ Header                    195           4615           6149          11115
C                                74           3383           2789          10162
reStructuredText                131           9313          10946           9282
CMake                           164            653            350           2195
Perl                             41            455            151           1925
Standard ML                       6              0              0           1555
YAML                             14            225             98           1123
Bourne Shell                     20            240            540            827
JavaScript                        1             20              1            383
Markdown                          7            159              0            343
XML                               1              5              7            182
CSS                               1             10              1             45
SQL Data                          1              5              0             41
Bourne Again Shell                1              6             16             22
HTML                              2              2              0             18
--------------------------------------------------------------------------------
SUM:                           1092          48255          31727         155118
--------------------------------------------------------------------------------

With this PR:

cloc f59716308a
    1706 text files.
    1699 unique files.
     626 files ignored.

github.com/AlDanial/cloc v 1.90  T=0.49 s (2213.0 files/s, 478090.6 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
PO File                           8          21019            598          62727
SQL                             319           4822           6774          40403
C++                             108           3478           3636          13677
C/C++ Header                    195           4647           6042          11226
reStructuredText                131           9318          10944           9296
C                                64           2910           2357           8520
CMake                           164            651            350           2186
Perl                             41            455            151           1925
Standard ML                       6              0              0           1555
YAML                             14            225             98           1123
Bourne Shell                     20            240            540            827
JavaScript                        1             20              1            383
Markdown                          6            154              0            329
XML                               1              5              7            182
CSS                               1             10              1             45
SQL Data                          1              5              0             41
Bourne Again Shell                1              6             16             22
HTML                              2              2              0             18
--------------------------------------------------------------------------------
SUM:                           1083          47967          31515         154485
--------------------------------------------------------------------------------

Summary:

Files develop -> this PR change
SUM: 1092 -> 1083 -9
C 74 -> 64 -10
C++ 106 -> 108 +2
C/C++ Header 195 -> 195 0
Markdown 195 -> 195 0
Code develop -> this PR change
SUM: 155118 -> 154485 -633
C 10162 -> 8520 -1642
C++ 12770 -> 13677 +907
C/C++ Header 11115 -> 11226 +111
Other develop -> this PR change
blank 48255 -> 47967 -288
comment 31727 -> 31515 -212

TODO

  • Verify that @robe2 can build the windows package
  • PSC motion for removing support for MSVC
  • If yes:
    • final refinement of the code
    • Remove the windows CI

todo or not todo:

  • If yes, these is one or the other: (might be included in the motion)
  • Leave the door open for MSVC compilation
    • maybe they do want to apply the patch
  • Completely remove from CMakeLists MSVC

@pgRouting/admins

@robe2
Copy link
Member

robe2 commented Apr 6, 2023

I tested on my new msys2/mingw64 chain running gcc.exe (Rev10, Built by MSYS2 project) 12.2.0
and it compiles fine. I still need to test the pgTap tests, but my perlTAP thing is hanging on installing the perl TAP modules.

@cvvergara cvvergara marked this pull request as draft April 6, 2023 19:37
@cvvergara
Copy link
Member Author

I tested on my new msys2/mingw64 chain running gcc.exe (Rev10, Built by MSYS2 project) 12.2.0 and it compiles fine. I still need to test the pgTap tests, but my perlTAP thing is hanging on installing the perl TAP modules.

Cool, so now I am in the refinement process.
(That is why I converted to draft)

@cvvergara
Copy link
Member Author

@cvvergara cvvergara marked this pull request as ready for review May 4, 2023 12:50
@cvvergara cvvergara merged commit ad59265 into pgRouting:develop May 4, 2023
10 checks passed
@cvvergara
Copy link
Member Author

Closes #2494

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants