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

Cmap add test gdamiand #8737

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gdamiand
Copy link
Member

Summary of Changes

Solve a bug for CMap and GMap when we iterate on an empty map with an iterator that needs to mark cells, and when using index version.

Release Management

  • Affected package(s): CMap GMap

@sloriot
Copy link
Member

sloriot commented Feb 13, 2025

master or release branch?

@gdamiand
Copy link
Member Author

Maybe it is better for release branch? (but the bug is very specific thus it is ok to use master)

Comment on lines +89 to +90
if(this->cont())
{ mark_cell<Map,i,dim>(amap, adart, mcell_mark_number); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to have the test inside the function (as this is anyway provided) and this will in one go protect all calls to the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

the mark_cell function takes a descriptor as input; and has a requirement that this descriptor is "valid".
thus I think the current use is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants