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.

Sign up for GitHub

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

Jump to bottom

Add a commande line option to osrm-routed for max locations supported in distance table query #1298

Closed
wants to merge 5 commits into from
Closed

Add a commande line option to osrm-routed for max locations supported in distance table query #1298

wants to merge 5 commits into from

Conversation

frodrigo
Copy link
Member

Due to issue (with gcc or luabind depends on computer) to compile develop branch this patch is not tested on this branch. Nevertheless this patch work on last release. #1236

@DennisOSRM
Copy link
Collaborator

Thanks for filing the PR.

I don't like adding another parameter to the c'tor. It's better to pass a configuration object (as the sole parameter) to the c'tor that contains the other information.

@frodrigo
Copy link
Member Author

Can you initialize this configuration objet, please ?

@frodrigo
Copy link
Member Author

Can you at least tell me what you want more explicitly ?

@TheMarex
Copy link
Member

Can you initialize this configuration objet, please ?

What do you mean by that? How the struct should look like?

struct ServerConfig {
    ServerPaths paths;
    bool use_shared_memory;
    int max_locations_distance_table;
}

@DennisOSRM
Copy link
Collaborator

any updates @frodrigo ?

@frodrigo
Copy link
Member Author

The struct usage is as you want ?

@DennisOSRM
Copy link
Collaborator

Haven't looked at the code yet. @frodrigo did you implement it with the struct as suggested by @TheMarex?

@frodrigo
Copy link
Member Author

Yes, almost like suggested.

@TheMarex
Copy link
Member

TheMarex commented Jan 4, 2015

@frodrigo Added some comments to the commits. Once this is fixed this should be good to merge. :-)

@frodrigo frodrigo force-pushed the max_locations_distance_table branch from f053cd4 to ac6a94d Compare January 5, 2015 14:48
@frodrigo
Copy link
Member Author

frodrigo commented Jan 5, 2015

I did what you asked

@frodrigo frodrigo force-pushed the max_locations_distance_table branch from ac6a94d to 17bfa39 Compare January 5, 2015 16:33
@frodrigo
Copy link
Member Author

frodrigo commented Jan 5, 2015

Fix again with stash

@TheMarex
Copy link
Member

TheMarex commented Jan 5, 2015

Looks good! Once the build + tests pass I will merge this. Thanks again @frodrigo!

DennisOSRM
DennisOSRM reviewed Jan 5, 2015
View reviewed changes
Include/osrm/ServerConfig.h
@@ -0,0 +1,51 @@
/*

Copyright (c) 2014, Project OSRM, Dennis Luxen, others
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2015

@DennisOSRM
Copy link
Collaborator

Added some comments to the code.

@TheMarex hold with merging until the next release is tagged this week. The patch breaks the API that the node bindings use.

@TheMarex
Copy link
Member

TheMarex commented Jan 5, 2015

@DennisOSRM exactly, I need this option for node-osrm. So it would be best if we merge this pre-tag, right?

@DennisOSRM
Copy link
Collaborator

@TheMarex can it wait for a day or two?

DennisOSRM
DennisOSRM reviewed Jan 6, 2015
View reviewed changes
Util/ProgramOptions.h
@@ -272,6 +276,11 @@ inline unsigned GenerateServerProgramOptions(const int argc,
{
return INIT_OK_START_ENGINE;
}
if (1 > max_locations_distance_table)
{
throw OSRMException("Max location for distance table must be a positive number");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try using osrm::exception.

@DennisOSRM DennisOSRM mentioned this pull request Jan 6, 2015
Closed
@frodrigo frodrigo force-pushed the max_locations_distance_table branch from 17bfa39 to 12f2acc Compare January 6, 2015 15:33
@frodrigo
Copy link
Member Author

frodrigo commented Jan 8, 2015

It's all right now ?

@DennisOSRM
Copy link
Collaborator

nope, the CI fail. As mentioned above, replace OSRMException with osrm::exception.

@frodrigo
Copy link
Member Author

frodrigo commented Jan 8, 2015

I use osrm::exception in last commit

@DennisOSRM
Copy link
Collaborator

triggered a rebuild of the PR

@DennisOSRM
Copy link
Collaborator

something is fishy with this PR. Did you rebase onto latest develop branch?

@DennisOSRM DennisOSRM mentioned this pull request Jan 8, 2015
Merged
@DennisOSRM
Copy link
Collaborator

No idea what happened to this PR. Went ahead, merged manually into a new branch + PR: #1335

Will handle it from there and merge once the CIs come back positive.

@DennisOSRM DennisOSRM closed this Jan 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frodrigo @DennisOSRM @TheMarex