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

Remove paftl from point.h (and thus replace pqmap with std::map) #134

Merged

Conversation

pklampros
Copy link
Collaborator

No description provided.

@pklampros pklampros requested a review from blsemo March 7, 2018 14:17
Copy link
Collaborator

@blsemo blsemo left a comment

Choose a reason for hiding this comment

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

Looks fine overall, but a few issues to be addressed before merging.

// these lines get turned into "blocks" based by a tanify function based on q and the centre given
// above. Given q=4 and centre 1,1 this line will be from 0.625 to something bigger than 1
lines.add(1, Line(Point2f(0.5, 0.2), Point2f(0.5, 0.7)));
lines.insert(std::make_pair(1, Line(Point2f(0.5, 0.2), Point2f(0.5, 0.7))));
Copy link
Collaborator

Choose a reason for hiding this comment

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

lines[1] = Line(Point2f(0.5, 0.2), Point2f(0.5, 0.7)); is easier to read and achieves the same in this case.

pt.m_lines.remove_at(k);
}
std::map<int, Line>::iterator iter = pt.m_lines.begin(), end = pt.m_lines.end();
for(; iter != end; ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably evaluate .end() in the for statement - depending on the stl implementation, erase might change where end is pointing to.

pqmap<int,Line> lines0;
for (size_t m = 0; m < getPoint(curs).m_lines.size(); m++)
std::map<int,Line> lines0;
for (std::pair<int,Line> line: getPoint(curs).m_lines)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be a reference, not a copy of the pair

@pklampros pklampros merged commit 949238d into SpaceGroupUCL:master Mar 7, 2018
@pklampros pklampros deleted the cleanup_remove_pqmap_point_h branch March 7, 2018 16:58
@pklampros pklampros added this to the 0.7.0 milestone Mar 17, 2018
pklampros added a commit that referenced this pull request Sep 2, 2020
Remove paftl from point.h (and thus replace pqmap with std::map)
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.

2 participants