Skip to content

[libc++][test] Fix simple warnings - #74186

Merged
ldionne merged 4 commits into
llvm:mainfrom
StephanTLavavej:stl-15-simple-warnings
Dec 5, 2023
Merged

ldionne merged 4 commits into
llvm:mainfrom
StephanTLavavej:stl-15-simple-warnings

Conversation

@StephanTLavavej

@StephanTLavavej StephanTLavavej commented Dec 2, 2023 •

Copy link
Copy Markdown
Member

Found while running libc++'s tests with MSVC's STL.

This contains 3 commits to fix simple warnings, and 1 cleanup commit.

  • Add void-casts to fix -Wunused-variable warnings.
    • Something about MSVC's STL or our test environment makes Clang/LLVM emit warnings here. As the variables are intentionally unused, void-casting is the simplest way to acknowledge this.
  • Avoid sign/truncation warnings in ConvertibleToIntegral.h.
    • As these warnings are emitted purely within test support code, we can silence them with static_cast without worrying about disrupting any usage. Examples:
      libcxx\test\std\containers\views\mdspan\layout_stride\../ConvertibleToIntegral.h(19): warning C4242: 'return': conversion from 'const int' to 'unsigned char', possible loss of data        
      libcxx\test\std\containers\views\mdspan\layout_stride\../ConvertibleToIntegral.h(20): warning C4242: 'return': conversion from 'const int' to 'signed char', possible loss of data
      libcxx\test\std\containers\views\mdspan\layout_stride\../ConvertibleToIntegral.h(32): warning C4242: 'return': conversion from 'int' to 'char', possible loss of data
      
  • Add TEST_STD_AT_LEAST_23_OR_RUNTIME_EVALUATED to avoid mixing preprocessor and runtime tests.
    • When MSVC sees if (TEST_STD_VER >= 23 || !TEST_IS_CONSTANT_EVALUATED) it emits warning C4127: conditional expression is constant because the first part is truly a compile-time constant. To fix this, I'm mechanically simplifying it into a centralized macro.
  • Cleanup: Add TEST_STD_AT_LEAST_20_OR_RUNTIME_EVALUATED for consistency.
    • The codebase had the same pattern, but with C++20. Although these didn't seem to be causing warnings for MSVC's STL, using the new pattern makes it more likely that it'll be followed for other code (including C++26 and beyond).

@StephanTLavavej
StephanTLavavej requested a review from a team as a code owner December 2, 2023 07:24
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 2, 2023
@llvmbot

llvmbot commented Dec 2, 2023

Copy link
Copy Markdown
Member

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

Found while running libc++'s tests with MSVC's STL.

This contains 3 small commits to fix simple warnings; I hope that combining them into a single PR makes things easier.

  • Avoid mixing preprocessor and runtime tests.
    • When MSVC sees if (TEST_STD_VER >= 23 || !TEST_IS_CONSTANT_EVALUATED) it emits warning C4127: conditional expression is constant because the first part is truly a compile-time constant. Transforming this logic avoids the warning. For safety, I'm adding braces to the controlled statement.
  • Add void-casts to fix -Wunused-variable warnings.
    • Something about MSVC's STL or our test environment makes Clang/LLVM emit warnings here. As the variables are intentionally unused, void-casting is the simplest way to acknowledge this.
  • Avoid sign/truncation warnings in ConvertibleToIntegral.h.
    • As these warnings are emitted purely within test support code, we can silence them with static_cast without worrying about disrupting any usage. Examples:
      libcxx\test\std\containers\views\mdspan\layout_stride\../ConvertibleToIntegral.h(19): warning C4242: 'return': conversion from 'const int' to 'unsigned char', possible loss of data        
      libcxx\test\std\containers\views\mdspan\layout_stride\../ConvertibleToIntegral.h(20): warning C4242: 'return': conversion from 'const int' to 'signed char', possible loss of data
      libcxx\test\std\containers\views\mdspan\layout_stride\../ConvertibleToIntegral.h(32): warning C4242: 'return': conversion from 'int' to 'char', possible loss of data
      

Full diff: https://github.com/llvm/llvm-project/pull/74186.diff

6 Files Affected:

  • (modified) libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move.pass.cpp (+6-1)
  • (modified) libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move_backward.pass.cpp (+6-1)
  • (modified) libcxx/test/std/containers/views/mdspan/ConvertibleToIntegral.h (+4-4)
  • (modified) libcxx/test/std/numerics/rand/rand.device/ctor.pass.cpp (+1)
  • (modified) libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.scoped/mutex.pass.cpp (+1)
  • (modified) libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.shared_mutex.requirements/thread.shared_mutex.class/default.pass.cpp (+1)
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move.pass.cpp
index ce5cf0560fc81ae..e6019c477f93e27 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move.pass.cpp
@@ -94,8 +94,13 @@ struct Test1OutIters {
 
 TEST_CONSTEXPR_CXX20 bool test() {
   types::for_each(types::cpp17_input_iterator_list<int*>(), TestOutIters());
-  if (TEST_STD_VER >= 23 || !TEST_IS_CONSTANT_EVALUATED)
+
+#if TEST_STD_VER < 23
+  if (!TEST_IS_CONSTANT_EVALUATED)
+#endif
+  {
     types::for_each(types::cpp17_input_iterator_list<std::unique_ptr<int>*>(), Test1OutIters());
+  }
 
   { // Make sure that padding bits aren't copied
     Derived src(1, 2, 3);
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move_backward.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move_backward.pass.cpp
index 2ed4d37b9dbe645..dcee48244288bfe 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move_backward.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move_backward.pass.cpp
@@ -92,8 +92,13 @@ struct Test1OutIters {
 
 TEST_CONSTEXPR_CXX20 bool test() {
   types::for_each(types::bidirectional_iterator_list<int*>(), TestOutIters());
-  if (TEST_STD_VER >= 23 || !TEST_IS_CONSTANT_EVALUATED)
+
+#if TEST_STD_VER < 23
+  if (!TEST_IS_CONSTANT_EVALUATED)
+#endif
+  {
     types::for_each(types::bidirectional_iterator_list<std::unique_ptr<int>*>(), Test1OutIters());
+  }
 
   { // Make sure that padding bits aren't copied
     Derived src(1, 2, 3);
diff --git a/libcxx/test/std/containers/views/mdspan/ConvertibleToIntegral.h b/libcxx/test/std/containers/views/mdspan/ConvertibleToIntegral.h
index 470f5d8e724648c..0ca5c330a390e4b 100644
--- a/libcxx/test/std/containers/views/mdspan/ConvertibleToIntegral.h
+++ b/libcxx/test/std/containers/views/mdspan/ConvertibleToIntegral.h
@@ -16,8 +16,8 @@ struct IntType {
 
   constexpr bool operator==(const IntType& rhs) const { return val == rhs.val; }
   constexpr operator int() const noexcept { return val; }
-  constexpr operator unsigned char() const { return val; }
-  constexpr operator signed char() const noexcept { return val; }
+  constexpr operator unsigned char() const { return static_cast<unsigned char>(val); }
+  constexpr operator signed char() const noexcept { return static_cast<signed char>(val); }
 };
 
 // only non-const convertible
@@ -28,8 +28,8 @@ struct IntTypeNC {
 
   constexpr bool operator==(const IntType& rhs) const { return val == rhs.val; }
   constexpr operator int() noexcept { return val; }
-  constexpr operator unsigned() { return val; }
-  constexpr operator char() noexcept { return val; }
+  constexpr operator unsigned() { return static_cast<unsigned>(val); }
+  constexpr operator char() noexcept { return static_cast<char>(val); }
 };
 
 // weird configurability of convertibility to int
diff --git a/libcxx/test/std/numerics/rand/rand.device/ctor.pass.cpp b/libcxx/test/std/numerics/rand/rand.device/ctor.pass.cpp
index a2d46ab1b94c7a5..796ab41716dd5f4 100644
--- a/libcxx/test/std/numerics/rand/rand.device/ctor.pass.cpp
+++ b/libcxx/test/std/numerics/rand/rand.device/ctor.pass.cpp
@@ -61,6 +61,7 @@ void check_random_device_invalid(const std::string &token) {
 int main(int, char**) {
   {
     std::random_device r;
+    (void)r;
   }
   // Check the validity of various tokens
   {
diff --git a/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.scoped/mutex.pass.cpp b/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.scoped/mutex.pass.cpp
index 48a96f90254e3e3..f953fa4f8d6df20 100644
--- a/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.scoped/mutex.pass.cpp
+++ b/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.scoped/mutex.pass.cpp
@@ -66,6 +66,7 @@ int main(int, char**)
     {
         using LG = std::scoped_lock<>;
         LG lg;
+        (void)lg;
     }
     {
         using LG = std::scoped_lock<TestMutex>;
diff --git a/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.shared_mutex.requirements/thread.shared_mutex.class/default.pass.cpp b/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.shared_mutex.requirements/thread.shared_mutex.class/default.pass.cpp
index 91320a52b62aea6..6b4da89c4f2d491 100644
--- a/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.shared_mutex.requirements/thread.shared_mutex.class/default.pass.cpp
+++ b/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.shared_mutex.requirements/thread.shared_mutex.class/default.pass.cpp
@@ -22,6 +22,7 @@
 int main(int, char**)
 {
     std::shared_mutex m;
+    (void)m;
 
   return 0;
 }

@github-actions

github-actions Bot commented Dec 2, 2023 •

Copy link
Copy Markdown

✅ With the latest revision this PR passed the C/C++ code formatter.

Comment thread libcxx/test/std/algorithms/alg.modifying.operations/alg.move/move.pass.cpp Outdated
@StephanTLavavej

This comment was marked as resolved.

Also apply clang-format from CI.
Examples:

libcxx\test\std\containers\views\mdspan\layout_stride\../ConvertibleToIntegral.h(19): warning C4242: 'return': conversion from 'const int' to 'unsigned char', possible loss of data
libcxx\test\std\containers\views\mdspan\layout_stride\../ConvertibleToIntegral.h(20): warning C4242: 'return': conversion from 'const int' to 'signed char', possible loss of data
libcxx\test\std\containers\views\mdspan\layout_stride\../ConvertibleToIntegral.h(32): warning C4242: 'return': conversion from 'int' to 'char', possible loss of data
@StephanTLavavej

Copy link
Copy Markdown
Member Author

Ok, I've got an even simpler approach that should work.

The history of reverts and reworks was getting too complicated, so I tidied it up, rebased, and force-pushed. Apologies if this complicates incremental reviewing (I think it should hopefully be easier to follow). Also updated the PR description.

@ldionne ldionne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a huge fan of the new macros because they won't compose super well, but it'll do. Thanks for the fix!

@ldionne
ldionne merged commit 164c204 into llvm:main Dec 5, 2023
@StephanTLavavej
StephanTLavavej deleted the stl-15-simple-warnings branch December 5, 2023 19:49
paleauraaaa pushed a commit to paleauraaaa/llvm-project that referenced this pull request Oct 7, 2026
Found while running libc++'s tests with MSVC's STL. This fixes 3 kinds of warnings:

- Add void-casts to fix `-Wunused-variable` warnings.
- Avoid sign/truncation warnings in `ConvertibleToIntegral.h`.
- Add `TEST_STD_AT_LEAST_23_OR_RUNTIME_EVALUATED` to avoid mixing preprocessor 
  and runtime tests.
- Cleanup: Add `TEST_STD_AT_LEAST_20_OR_RUNTIME_EVALUATED` for
  consistency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants