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

Improvements to ArrayView interface #221

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

adayton1
Copy link
Member

@adayton1 adayton1 commented Apr 11, 2023

  • Adds ArrayView aliases
  • Adds factory functions for making const views

Copy link
Member

@agcapps agcapps left a comment

Choose a reason for hiding this comment

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

Alan, these look like useful improvements.

using ArrayView1D = RAJA::View<T, RAJA::Layout<1>>;

template <class T>
using ArrayCView1D = ArrayView1D<const T>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is nit picky, but why did you call this ArrayCView1D rather than ArrayViewC1D (Array View Constant 1D) or CArrayView1D (Constant Array View 1D)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess elsewhere you call this a const view so maybe that makes sense nvm

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually going to ask what you think of the naming. Would it make sense to drop "Array" from the naming scheme and just have View1D, ConstView1D, View2D, ConstView2D, etc...?

Copy link
Collaborator

@dtaller dtaller Apr 11, 2023

Choose a reason for hiding this comment

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

That makes sense. We have RAJA::View, so having care::View would be consistent and it would be clear as long as it is in the care namespace. Spelling out Constant makes things clear also, harder to miss the difference. But I would ask some of the others before making a sweeping change.

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't used anywhere yet, so now is a great time to make changes.

Copy link
Collaborator

@dtaller dtaller left a comment

Choose a reason for hiding this comment

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

Looks good to me

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.

4 participants