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

Reuse sample::get implementation #3471

Merged
merged 5 commits into from
Jan 4, 2022
Merged

Reuse sample::get implementation #3471

merged 5 commits into from
Jan 4, 2022

Conversation

merkispavel
Copy link
Contributor

Issue

Previously we had duplicated logic for sample::get and sample::get_all. The PR

  • removes the duplication by adding helper method get(coord, tile&)(like we have in GraphReader)
  • minor: sets default tile_data index to invalid value(i.e TILE_COUNT instead of 0)

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@@ -212,7 +212,7 @@ class tile_data {
bool reusable;

public:
tile_data() : c(nullptr), index(0), reusable(false), data(nullptr) {
tile_data() : c(nullptr), index(TILE_COUNT), reusable(false), data(nullptr) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm not mistaken - index=0 is actually a valid index. So it's better to fill it with invalid index value

template <class coords_t> std::vector<double> sample::get_all(const coords_t& coords) {
std::vector<double> values;
values.reserve(coords.size());
uint16_t curIndex = TILE_COUNT;
Copy link
Contributor Author

@merkispavel merkispavel Dec 24, 2021

Choose a reason for hiding this comment

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

tile_data already stores index so I just added tile_data::get_index getter to obtain it

kevinkreiser
kevinkreiser previously approved these changes Jan 2, 2022
Copy link
Member

@kevinkreiser kevinkreiser left a comment

Choose a reason for hiding this comment

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

this looks good to me, do you mind adding that extra comment for clarity?

Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>
@merkispavel
Copy link
Contributor Author

merkispavel commented Jan 4, 2022

this looks good to me, do you mind adding that extra comment for clarity?

Sure thing! done
Thanks for reviewing it

@kevinkreiser kevinkreiser merged commit e763174 into master Jan 4, 2022
@nilsnolde nilsnolde deleted the reuse-sample-get branch February 24, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants