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

[libc] fix HashTable warnings and build problems #74371

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

SchrodingerZhu
Copy link
Contributor

@SchrodingerZhu SchrodingerZhu commented Dec 4, 2023

According to https://lab.llvm.org/buildbot/#/builders/163/builds/48002, the generic build on HashTable fails with two major issues with werror:

  1. warnings on error: suggest braces around initialization of subobject.
  2. __support/HashTable tests are built regardless of its entrypoints`

This PR attempts to fix such issues.

@llvmbot llvmbot added the libc label Dec 4, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 4, 2023

@llvm/pr-subscribers-libc

Author: Schrodinger ZHU Yifan (SchrodingerZhu)

Changes

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

3 Files Affected:

  • (modified) libc/src/__support/HashTable/bitmask.h (+1-1)
  • (modified) libc/src/__support/HashTable/generic/bitmask_impl.inc (+1-1)
  • (modified) libc/test/src/__support/CMakeLists.txt (+9-1)
diff --git a/libc/src/__support/HashTable/bitmask.h b/libc/src/__support/HashTable/bitmask.h
index 8247161c449bd..821735e22f0a4 100644
--- a/libc/src/__support/HashTable/bitmask.h
+++ b/libc/src/__support/HashTable/bitmask.h
@@ -73,7 +73,7 @@ template <class BitMask> struct IteratableBitMaskAdaptor : public BitMask {
     return *this;
   }
   LIBC_INLINE IteratableBitMaskAdaptor begin() { return *this; }
-  LIBC_INLINE IteratableBitMaskAdaptor end() { return {0}; }
+  LIBC_INLINE IteratableBitMaskAdaptor end() { return {{0}}; }
   LIBC_INLINE bool operator==(const IteratableBitMaskAdaptor &other) {
     return this->word == other.word;
   }
diff --git a/libc/src/__support/HashTable/generic/bitmask_impl.inc b/libc/src/__support/HashTable/generic/bitmask_impl.inc
index b8d2bfc7a6ff2..623b51f238df5 100644
--- a/libc/src/__support/HashTable/generic/bitmask_impl.inc
+++ b/libc/src/__support/HashTable/generic/bitmask_impl.inc
@@ -98,7 +98,7 @@ struct Group {
     auto cmp = data ^ repeat_byte(byte);
     auto result = LIBC_NAMESPACE::Endian::to_little_endian(
         (cmp - repeat_byte(0x01)) & ~cmp & repeat_byte(0x80));
-    return {result};
+    return {{result}};
   }
 
   // Find out the lanes equal to EMPTY or DELETE (highest bit set) and
diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index 2b9fa93bb548e..6db0e53055b25 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -181,4 +181,12 @@ add_subdirectory(File)
 add_subdirectory(RPC)
 add_subdirectory(OSUtil)
 add_subdirectory(FPUtil)
-add_subdirectory(HashTable)
+
+list(FIND TARGET_ENTRYPOINT_NAME_LIST hsearch hsearch_index)
+list(FIND TARGET_ENTRYPOINT_NAME_LIST hsearch_r hsearch_r_index)
+
+if (${hsearch_index} EQUAL -1 AND ${hsearch_r_index} EQUAL -1)
+  message(STATUS "Skipping HashTable tests since hsearch and hsearch_r are not built.")
+else()
+  add_subdirectory(HashTable)
+endif()

@@ -181,4 +181,12 @@ add_subdirectory(File)
add_subdirectory(RPC)
add_subdirectory(OSUtil)
add_subdirectory(FPUtil)
add_subdirectory(HashTable)

Copy link
Contributor

Choose a reason for hiding this comment

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

The tests are build after all the sources right? It should be easier to just check the target like if(TARGET libc.src.<whatever>.hsearch)

@@ -73,7 +73,7 @@ template <class BitMask> struct IteratableBitMaskAdaptor : public BitMask {
return *this;
}
LIBC_INLINE IteratableBitMaskAdaptor begin() { return *this; }
LIBC_INLINE IteratableBitMaskAdaptor end() { return {0}; }
LIBC_INLINE IteratableBitMaskAdaptor end() { return {{0}}; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the best way to silence the warning? Can we not explicitly default construct the type to make it clearer what's actually being done here?

@@ -181,4 +181,12 @@ add_subdirectory(File)
add_subdirectory(RPC)
add_subdirectory(OSUtil)
add_subdirectory(FPUtil)
add_subdirectory(HashTable)

list(FIND TARGET_ENTRYPOINT_NAME_LIST hsearch hsearch_index)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just put hsearch as a dependency of the test and it will automatically exclude it for you

@nickdesaulniers
Copy link
Member

Please put the diagnostic observed from the compiler in the PR description.

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

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

LGTM

@SchrodingerZhu
Copy link
Contributor Author

Please remember to use the updated PR description when merging this. Sometimes github don't automatically draw the updates into the default commit message.

@michaelrj-google michaelrj-google merged commit 6fd1c1b into llvm:main Dec 4, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants