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

gtest: add cast to remove sign-compare comparison warning #2433

Closed
wants to merge 1 commit into from

Conversation

ndellingwood
Copy link
Contributor

Resolve error: comparison of integer expressions of different signedness: 'const long unsigned int' and 'const long int' [-Werror=sign-compare]

The warning is triggering -Werror in Trilinos nightly integration builds

Resolve `error: comparison of integer expressions of different
signedness: 'const long unsigned int' and 'const long int'
[-Werror=sign-compare]`

The warning is triggering -Werror in Trilinos nightly integration builds
@ndellingwood
Copy link
Contributor Author

Follow on to #2416 (comment)

@ndellingwood
Copy link
Contributor Author

The clang-format check wants to reformat the entire gtest.h file, should gtest.h be excluded from the check?

@ndellingwood
Copy link
Contributor Author

The AT2 h100 check is failing with

 Unable to determine the device handle for GPU0000:B8:00.0: Unknown Error
Error: Process completed with exit code 255.

I'm not sure what that indicates, probably something I should forward to sys admins?

@cwpearson
Copy link
Contributor

@ndellingwood let's not format gtest. The way the format check is set up right now is that it tries to format all changed files, rather than specific files.

Copy link
Contributor

@cwpearson cwpearson left a comment

Choose a reason for hiding this comment

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

I'd rather put the cast in the test itself rather than modify gtest.

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.

Same as Carl, can you let us know which test triggers this compiler error?
Also I am somewhat surprised that this does not appear in the Kokkos Kernels CI directly? Let us know what combination of compiler flags and KK configuration triggers this failure so we can cover it better.

@ndellingwood
Copy link
Contributor Author

I'll close this, for reference this is the -Werror reported on cdash:

In file included from ../kokkos-kernels/test_common/Test_Serial.hpp:19,
                 from ../kokkos-kernels/sparse/unit_test/backends/Test_Serial_Sparse.cpp:19:
../kokkos-kernels/tpls/gtest/gtest/gtest.h: In instantiation of 'testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = long int]':
../kokkos-kernels/tpls/gtest/gtest/gtest.h:11446:23:   required from 'static testing::AssertionResult testing::internal::EqHelper::Compare(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = long int; typename std::enable_if<((! std::is_integral<_Tp>::value) || (! std::is_pointer<_Dp>::value))>::type* <anonymous> = 0]'
../kokkos-kernels/sparse/unit_test/Test_Sparse_TestUtils_RandCsMat.hpp:44:3:   required from 'void Test::doCsMat(size_t, size_t, ScalarType, ScalarType) [with ScalarType = float; LayoutType = Kokkos::LayoutLeft; ExeSpaceType = Kokkos::Serial; size_t = long unsigned int]'
../kokkos-kernels/sparse/unit_test/Test_Sparse_TestUtils_RandCsMat.hpp:64:51:   required from 'void Test::doAllCsMat(size_t, size_t) [with ExeSpaceType = Kokkos::Serial; size_t = long unsigned int]'
../kokkos-kernels/sparse/unit_test/Test_Sparse_TestUtils_RandCsMat.hpp:78:74:   required from here
../kokkos-kernels/tpls/gtest/gtest/gtest.h:11427:11: error: comparison of integer expressions of different signedness: 'const long unsigned int' and 'const long int' [-Werror=sign-compare]
   if (lhs == rhs) {
       ~~~~^~~~~~
[CTest: warning matched] cc1plus: some warnings being treated as errors

https://trilinos-cdash.sandia.gov/viewBuildError.php?buildid=1994256

@cwpearson
Copy link
Contributor

cwpearson commented Nov 26, 2024

The offending line is

ASSERT_EQ(cm.get_nnz(), expected_nnz) << cm.info;

Where lhs is a long unsigned int coming from KokkosKernels::default_size_type and rhs is a long int, which comes from RandCsMatrix::ordinal_type, which is defaulted to int64_t in this scenario.

This strikes me as a common case, I'm not sure why we don't see it in our testing

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