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

Fix warnings (shadow and conversion) #340

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

crtrott
Copy link
Member

@crtrott crtrott commented Jun 4, 2024

The extent and stride thing shadow something new in utility.hpp

@crtrott crtrott changed the title Fix warnings (shadown and conversion) Fix warnings (shadow and conversion) Jun 4, 2024
Copy link
Contributor

@mhoemmen mhoemmen left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! : - )

@@ -651,15 +651,15 @@ constexpr void validate_strides(with_rank<N>, Layout, const Extents& ext, const

constexpr auto is_left = std::is_same<Layout, layout_left>::value;

typename Extents::index_type stride = 1;
typename Extents::index_type stride_ = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider avoiding use of trailing underscore for variables that aren't nonpublic data members of the class?

Suggested change
typename Extents::index_type stride_ = 1;
typename Extents::index_type the_stride = 1;

include/experimental/__p0009_bits/layout_stride.hpp Outdated Show resolved Hide resolved
and "invalid strides for layout_{left,right}");

stride *= ext.extent(s);
stride_ *= ext.extent(s);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stride_ *= ext.extent(s);
the_stride *= ext.extent(s);

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to expected_stride

@nmm0
Copy link
Contributor

nmm0 commented Jun 5, 2024

I don't think our CI is actually testing Wshadow

@masterleinad
Copy link
Contributor

I don't think our CI is actually testing Wshadow

Looks like the GitHub Workflows don't test with any warning flags and https://github.com/kokkos/mdspan/blob/stable/README.md#running-tests mentions that a lot of the configurations would not build with warnings as errors (depending on the C++ standard).

@crtrott
Copy link
Member Author

crtrott commented Jun 6, 2024

I don't think our CI is actually testing Wshadow

I know. But we can touch CI separately.

Copy link
Contributor

@nmm0 nmm0 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!

@crtrott crtrott merged commit 96b3985 into kokkos:stable Jun 6, 2024
14 checks passed
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