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

Added NodeList index to DataBase #313

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

brbass
Copy link
Collaborator

@brbass brbass commented Nov 6, 2024

This lets us calculate the index in the DataBase that represents a given NodeList in the connectivity.

ToDo :

  • Annotate RELEASE_NOTES.md with notable changes.
  • Create LLNLSpheral PR pointing at this branch. (PR#)
  • LLNLSpheral PR has passed all tests.

jmikeowen
jmikeowen previously approved these changes Nov 7, 2024
Copy link
Collaborator

@jmikeowen jmikeowen 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 fine. A heads up for the future -- I've been slowly transitioning things like this from int to size_t, which I now know is the more appropriate type for sizes and indices into containers like this. The int here is fine for now though, and consistent with the other things that return int's in the DataBase interface.

@mdavis36
Copy link
Collaborator

@brbass I know you didn't want to create a separate LLNL PR for this. However, if you could have #PR113 point to this branch we can consider the LLNLSpheral requirement for this PR satisfied when both pass.

@brbass
Copy link
Collaborator Author

brbass commented Nov 19, 2024

@brbass I know you didn't want to create a separate LLNL PR for this. However, if you could have #PR113 point to this branch we can consider the LLNLSpheral requirement for this PR satisfied when both pass.

That MR is pointing at this one already. We should wait until that one is ready in case I need to make other changes.

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.

3 participants