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

deprecate KOKKOS_CUSPARSE_SAFE_CALL -> KOKKOSPARSE_IMPL_CUSPARSE_SAFE_CALL #2426

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

Conversation

cwpearson
Copy link
Contributor

Fix for #2371

Copy link

@JBludau JBludau left a comment

Choose a reason for hiding this comment

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

can you paste the command you used for the search and replace?

Comment on lines 223 to 229
ArrayType& operator=(const ArrayType& rhs_) {
if (this != &rhs_) {
for (size_type i = 0; i < MAX_VEC_SIZE; ++i) {
m_data[i] = rhs_.m_data[i];
}
ArrayType &operator=(const ArrayType &rhs_) {
if (this != &rhs_) {
for (size_type i = 0; i < MAX_VEC_SIZE; ++i) {
m_data[i] = rhs_.m_data[i];
}
return *this;
}
return *this;
Copy link

Choose a reason for hiding this comment

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

what happened here?

Copy link

Choose a reason for hiding this comment

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

ok, maybe this is a clang-format/git artifact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this is formatting related, I’m not sure why it wasn’t formatted before.

@lucbv
Copy link
Contributor

lucbv commented Nov 11, 2024

Something strange going on in the mi210 without TPLs, the tests are all reporting a segfault.
This might be a machine/workflow issue rather than an actual bug...

Copy link
Contributor

@lucbv lucbv left a comment

Choose a reason for hiding this comment

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

Okay, this looks reasonable to me.
As mentioned by Jacob, have the command line/script in the PR description would be a good idea : )

@cwpearson
Copy link
Contributor Author

I used Visual Studio Code’s global search/replace, except for the area where I implemented the deprecation macro.

Comment on lines 67 to 68
// The macro below defines is the public interface for the safe cusparse calls.
// The functions themselves are protected by impl namespace.
Copy link

Choose a reason for hiding this comment

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

I think the comment should be revised.
I could not find any documentation of the macro as part of the public interface. Do you know of anyone using it? Do you want to have the old one around but emit a deprecation warning instead of removing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I somehow missed pushing the deprecation part, it's in now.

@cwpearson
Copy link
Contributor Author

@cwpearson cwpearson force-pushed the fix/2371 branch 4 times, most recently from d8bac27 to be73bc1 Compare November 13, 2024 15:14
Copy link

@JBludau JBludau 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. Lets see how much trilinos is going to complain

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