baldr: Adjusting code to C++17

[cvvergara]

Issue

baldr part of #4018

TODO

• removing the superfluous try catch from the new edgeinfo function
• correcting the docstrings for the edgeinfo functions
• making a sentinel invalid function that is properly typed to replace uses of -1 (probably goes in midgard/util headers or somewhere everything can use it)
• fixing the system include directive to work in the valhalla_module so we can keep date stuff in baldr

[cvvergara]

In this commit:
9440737
all files on sources_with_warnings are moved to sources
No warning on baldr should appear on the CI when this PR is ready for review.

[cvvergara]

@kevinkreiser I think this is ready.

[kevinkreiser]

sounds good! ill take another look hopefully this evening, sorry again for the delay and thanks again for grinding th rough it!

[cvvergara]

@kevinkreiser made the changes

[kevinkreiser]

same casting here for same reason

[kevinkreiser]

the previous code was a bug because someone could screw with the version string and make it shorter than kMaxVersionSize and strncpy would have read past the end, so its good you are fixing it however i think you have a new bug here, this should be:

Suggested change

	std::copy(version.begin(), version.begin() + std::min(kMaxVersionSize, sizeof(version.c_str())),
	std::copy(version.begin(), version.begin() + std::min(kMaxVersionSize, version.size()),

otherwise it will only ever copy 8 bytes because c_str() is a pointer which is 64bits wide (these days)

[cvvergara]

nice catch :-)

[cvvergara]

@kevinkreiser made the changes

[kevinkreiser]

ok i made it to the bottom of the diff! the only changes i would request are:

1. removing the superfluous try catch from the new edgeinfo function
2. correcting the docstrings for the edgeinfo functions
3. making a sentinel invalid function that is properly typed to replace uses of -1 (probably goes in midgard/util headers or somewhere everything can use it)
4. fixing the system include directive to work in the valhalla_module so we can keep date stuff in baldr

[kevinkreiser]

regarding number 4, i tried it out myself and it just worked for me, this was my diff with master:

diff --git a/src/baldr/CMakeLists.txt b/src/baldr/CMakeLists.txt
index 1ce31ea6d..306646f26 100644
--- a/src/baldr/CMakeLists.txt
+++ b/src/baldr/CMakeLists.txt
@@ -30,7 +30,6 @@ set(includes
      ${VALHALLA_SOURCE_DIR}/valhalla
      $<$<BOOL:${WIN32}>:${VALHALLA_SOURCE_DIR}/third_party/dirent/include>
      ${VALHALLA_SOURCE_DIR}/third_party/rapidjson/include
-     ${VALHALLA_SOURCE_DIR}/third_party/date/include
      ${CMAKE_CURRENT_BINARY_DIR}/src/baldr
      )
 
@@ -114,6 +113,8 @@ valhalla_module(NAME baldr
       ${includes}
     PRIVATE
       ${CMAKE_CURRENT_BINARY_DIR}
+  SYSTEM
+      ${VALHALLA_SOURCE_DIR}/third_party/date/include
 
   DEPENDS
     valhalla::midgard

now in reality it should be slightly more complicated because of the ios includes i guess but only very slightly more complicated. also, did you know that clion now lets you debug cmake scripts!? its awesome you can litterally step through a cmake run in the debugger and watch varaibles and general flow through the script, pretty awesome!

[cvvergara]

regarding number 4, i tried it out myself and it just worked for me, this was my diff with master:

diff --git a/src/baldr/CMakeLists.txt b/src/baldr/CMakeLists.txt
index 1ce31ea6d..306646f26 100644
--- a/src/baldr/CMakeLists.txt
+++ b/src/baldr/CMakeLists.txt
@@ -30,7 +30,6 @@ set(includes
      ${VALHALLA_SOURCE_DIR}/valhalla
      $<$<BOOL:${WIN32}>:${VALHALLA_SOURCE_DIR}/third_party/dirent/include>
      ${VALHALLA_SOURCE_DIR}/third_party/rapidjson/include
-     ${VALHALLA_SOURCE_DIR}/third_party/date/include
      ${CMAKE_CURRENT_BINARY_DIR}/src/baldr
      )
 
@@ -114,6 +113,8 @@ valhalla_module(NAME baldr
       ${includes}
     PRIVATE
       ${CMAKE_CURRENT_BINARY_DIR}
+  SYSTEM
+      ${VALHALLA_SOURCE_DIR}/third_party/date/include
 
   DEPENDS
     valhalla::midgard

now in reality it should be slightly more complicated because of the ios includes i guess but only very slightly more complicated. also, did you know that clion now lets you debug cmake scripts!? its awesome you can litterally step through a cmake run in the debugger and watch varaibles and general flow through the script, pretty awesome!

Doing that is leading to this in my computer (doing make VERBOSE=ON):

-I/home/vicky/valhalla/cvvergara/src/baldr/SYSTEM -I/home/vicky/valhalla/cvvergara/third_party/date/include

instead of:

-isystem /home/vicky/valhalla/cvvergara/third_party/date/include

Update, I didnt see the indentation, does not work :-(

[cvvergara]

ok i left a fresh round of review comments after going over the whole thing again. all of them are trivial so i suspect we can merge this soon?

Yes

[kevinkreiser]

we missed the one update to the docstring so i just commited it and since its only a comment i will just merge no need to wait for build