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

[search] Use better types for indices: int -> index_t, std::vector<int> ->Indices #3840

Merged
merged 2 commits into from
Apr 13, 2020

Conversation

mvieth
Copy link
Member

@mvieth mvieth commented Apr 1, 2020

No description provided.

search/include/pcl/search/brute_force.h Show resolved Hide resolved
search/include/pcl/search/impl/brute_force.hpp Outdated Show resolved Hide resolved
search/include/pcl/search/organized.h Outdated Show resolved Hide resolved
search/include/pcl/search/search.h Show resolved Hide resolved
@kunaltyagi kunaltyagi changed the title Use index_t and Indices definitions in module search [search] Use better types for indices: int -> index_t, std::vector<int> ->Indices Apr 1, 2020
@kunaltyagi kunaltyagi added changelog: enhancement Meta-information for changelog generation module: search needs: author reply Specify why not closed/merged yet labels Apr 1, 2020
Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

LGTM besides from the inline comments.

search/include/pcl/search/search.h Show resolved Hide resolved
search/include/pcl/search/search.h Show resolved Hide resolved
@kunaltyagi
Copy link
Member

Some files still have the removed type alias.

@taketwo Which option is better:

  • keep the type-alias in the class (referring to the global type alias): Current patch + some places that still need updating
  • keep the type-alias in the class (referring to the type alias in PCLBase): Needs some work, but is more uniform in the sense of not using the global alias everywhere
  • to deprecate it in favor of pcl::IndicesPtr: Needs deprecation, but is more uniform in the sense of using the global alias everywhere

@kunaltyagi kunaltyagi added needs: more work Specify why not closed/merged yet needs: feedback Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet needs: more work Specify why not closed/merged yet labels Apr 3, 2020
@taketwo
Copy link
Member

taketwo commented Apr 5, 2020

index_t is global, Indices is global, so it only makes sense to me that IndicesPtr is global as well. What's your opinion?

@kunaltyagi
Copy link
Member

Strictly speaking IndicesPtr is an implementation detail of the classes. On the other hand, it is very prevalent. Conclusion: anything works.

@taketwo
Copy link
Member

taketwo commented Apr 6, 2020

IndicesPtr is an implementation detail of the classes

In which sense? IndicesPtr is in the API of every PCL class because of the setIndices()/getIndices() methods inherited from PCLBase. So it's globally used, it's the same in every class, and users need to be aware of it.

@kunaltyagi
Copy link
Member

In which sense?

If I were to change the type in PCLBase, global IndicesPtr will need to be changed. This is a technicality only, and due to all classes being related to PCLBase in one way or another, having it global isn't bad, or to quote myself: "anything works".

The choices I listed were:

  • Use IndicesPtr as global: 2 votes for, agreed upon
  • Deprecate (or not) the IndicesPtr defined inside the classes: needs decision upon

@taketwo
Copy link
Member

taketwo commented Apr 13, 2020

Is there anything left to be done here?

@kunaltyagi
Copy link
Member

Nope. The uniformity across all classes can be addressed later. I have an idea of how to do that.

Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

Please squash 1st and 3rd commits.

@mvieth
Copy link
Member Author

mvieth commented Apr 13, 2020

@taketwo done

@kunaltyagi kunaltyagi merged commit b155d34 into PointCloudLibrary:master Apr 13, 2020
@SergioRAgostinho SergioRAgostinho removed the needs: feedback Specify why not closed/merged yet label Apr 14, 2020
@mvieth mvieth deleted the search_indices branch April 14, 2020 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants