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

unstdify config.hpp macros #366

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

Conversation

nmm0
Copy link
Contributor

@nmm0 nmm0 commented Nov 26, 2024

I used the bugprone-reserved-identifier clang-tidy check to find these, and a global search/replace to fix them. I kept these changes isolated to ones declared in config.hpp and plan on doing more later. (As you can see, even just the config.hpp ones touch basically every file)

@@ -15,21 +15,25 @@
//@HEADER
#pragma once

#ifndef __has_include
# define __has_include(x) 0
#ifndef MDSPAN_IMPL_HAS_INCLUDE
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 switched this to our own macro rather than substituting in the standard one (since I think that could propagate a bug downstream if someone does #ifdef __has_include

Copy link
Contributor

Choose a reason for hiding this comment

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

This is much better; thank you! : - )

# endif
#endif

#ifndef __has_cpp_attribute
# define __has_cpp_attribute(x) 0
#ifndef MDSPAN_IMPL_HAS_CPP_ATTRIBUTE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

We shouldn't just remove the leading underscore, we need to inject something to denote that these macros are private, along the lines of what we do in Kokkos with the KOKKOS_IMPL_ prefix

@nmm0
Copy link
Contributor Author

nmm0 commented Dec 3, 2024

We shouldn't just remove the leading underscore, we need to inject something to denote that these macros are private, along the lines of what we do in Kokkos with the KOKKOS_IMPL_ prefix

@dalg24 ok, I can do MDSPAN_IMPL_

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