Refactors Existing Alternatives Architecture#4035
Conversation
daniel-j-h
force-pushed
the
refactor-alternatives
branch
from
3534951 to
9339457
Compare
daniel-j-h
force-pushed
the
refactor-alternatives
branch
from
9339457 to
e064a93
Compare
|
|
||
| void MakeResponse(const InternalRouteResult &raw_route, util::json::Object &response) const | ||
| { | ||
| return MakeResponse(raw_route, response); |
There was a problem hiding this comment.
This function calls itself infinitely.
Should this be return MakeResponse(InternalManyRoutesResult(raw_route), response); ?
There was a problem hiding this comment.
Hm the InternalRouteResult should just silently convert to a InternalManyRouteResult, at least that was the plan. Not sure you're right because then basically no test would have run through. I will make a pr to change it to your suggestion - it's much clearer. Thanks for flagging!
There was a problem hiding this comment.
I noticed because clang issues a warning 😀
There was a problem hiding this comment.
Your local Clang or Clang 4 on Travis? Which warning is it? Can we enable it and -Werror on Travis? I want to catch these things asap and it looks like this was a serious issue.
There was a problem hiding this comment.
/Users/danpat/mapbox/osrm-backend/include/engine/api/route_api.hpp:69:5: warning: all paths through this function will call itself [-Winfinite-recursion]
{
^
I'm using clang from my local XCode install:
$ clang --version
Apple LLVM version 8.1.0 (clang-802.0.42)
Target: x86_64-apple-darwin16.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
I have no idea how it maps to the upstream version, but it's from XCode 8.3.2.
3 participants
Splitting off some required alternatives refactoring work to already target master. Functionality-wise nothing changes - but it lifts some restrictions to move forward in my mld alternatives work:
#3905
Note: the API needs to know that our alternatives impl. is using the via-method and therefore does not support queries with via nodes (only s,t queries) - would require some more re-structuring. I made a hard cut there.