Skip to content

Conversation

@kaladron
Copy link
Contributor

Replace the custom my_streq() helper function with LIBC_NAMESPACE::strcmp(), simplifying the test code and making it more consistent with other integration tests in the codebase.

Changes:

  • Removed 18-line my_streq() helper function
  • Added #include "src/string/strcmp.h"
  • Updated string comparisons:
    • Null checks: Direct comparison with nullptr
    • Equality checks: ASSERT_EQ(strcmp(...), 0)
    • Inequality checks: ASSERT_NE(strcmp(...), 0)
  • Added libc.src.string.strcmp dependency in CMakeLists.txt

This reduces the test file from 49 to 31 lines while maintaining the same test coverage. The strcmp-based approach is cleaner and aligns with the pattern used in the environment_management branch tests (setenv_test, unsetenv_test, putenv_test).

Testing: Verified that all test assertions pass with the new implementation.

@kaladron kaladron self-assigned this Oct 12, 2025
@llvmbot llvmbot added the libc label Oct 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Oct 12, 2025

@llvm/pr-subscribers-libc

Author: Jeff Bailey (kaladron)

Changes

Replace the custom my_streq() helper function with LIBC_NAMESPACE::strcmp(), simplifying the test code and making it more consistent with other integration tests in the codebase.

Changes:

  • Removed 18-line my_streq() helper function
  • Added #include "src/string/strcmp.h"
  • Updated string comparisons:
    • Null checks: Direct comparison with nullptr
    • Equality checks: ASSERT_EQ(strcmp(...), 0)
    • Inequality checks: ASSERT_NE(strcmp(...), 0)
  • Added libc.src.string.strcmp dependency in CMakeLists.txt

This reduces the test file from 49 to 31 lines while maintaining the same test coverage. The strcmp-based approach is cleaner and aligns with the pattern used in the environment_management branch tests (setenv_test, unsetenv_test, putenv_test).

Testing: Verified that all test assertions pass with the new implementation.


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

2 Files Affected:

  • (modified) libc/test/integration/src/stdlib/CMakeLists.txt (+1)
  • (modified) libc/test/integration/src/stdlib/getenv_test.cpp (+13-32)
diff --git a/libc/test/integration/src/stdlib/CMakeLists.txt b/libc/test/integration/src/stdlib/CMakeLists.txt
index 1773d9fc9f0f5..caa2a0b96288e 100644
--- a/libc/test/integration/src/stdlib/CMakeLists.txt
+++ b/libc/test/integration/src/stdlib/CMakeLists.txt
@@ -12,6 +12,7 @@ add_integration_test(
     getenv_test.cpp
   DEPENDS
     libc.src.stdlib.getenv
+    libc.src.string.strcmp
   ENV
     FRANCE=Paris
     GERMANY=Berlin
diff --git a/libc/test/integration/src/stdlib/getenv_test.cpp b/libc/test/integration/src/stdlib/getenv_test.cpp
index 72dcea0ddf1f1..b909df94c7fb6 100644
--- a/libc/test/integration/src/stdlib/getenv_test.cpp
+++ b/libc/test/integration/src/stdlib/getenv_test.cpp
@@ -7,43 +7,24 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/stdlib/getenv.h"
+#include "src/string/strcmp.h"
 
 #include "test/IntegrationTest/test.h"
 
-static bool my_streq(const char *lhs, const char *rhs) {
-  if (lhs == rhs)
-    return true;
-  if (((lhs == static_cast<char *>(nullptr)) &&
-       (rhs != static_cast<char *>(nullptr))) ||
-      ((lhs != static_cast<char *>(nullptr)) &&
-       (rhs == static_cast<char *>(nullptr)))) {
-    return false;
-  }
-  const char *l, *r;
-  for (l = lhs, r = rhs; *l != '\0' && *r != '\0'; ++l, ++r)
-    if (*l != *r)
-      return false;
-
-  return *l == '\0' && *r == '\0';
-}
-
 TEST_MAIN([[maybe_unused]] int argc, [[maybe_unused]] char **argv,
           [[maybe_unused]] char **envp) {
-  ASSERT_TRUE(
-      my_streq(LIBC_NAMESPACE::getenv(""), static_cast<char *>(nullptr)));
-  ASSERT_TRUE(
-      my_streq(LIBC_NAMESPACE::getenv("="), static_cast<char *>(nullptr)));
-  ASSERT_TRUE(my_streq(LIBC_NAMESPACE::getenv("MISSING ENV VARIABLE"),
-                       static_cast<char *>(nullptr)));
-  ASSERT_FALSE(
-      my_streq(LIBC_NAMESPACE::getenv("PATH"), static_cast<char *>(nullptr)));
-  ASSERT_TRUE(my_streq(LIBC_NAMESPACE::getenv("FRANCE"), "Paris"));
-  ASSERT_FALSE(my_streq(LIBC_NAMESPACE::getenv("FRANCE"), "Berlin"));
-  ASSERT_TRUE(my_streq(LIBC_NAMESPACE::getenv("GERMANY"), "Berlin"));
-  ASSERT_TRUE(
-      my_streq(LIBC_NAMESPACE::getenv("FRANC"), static_cast<char *>(nullptr)));
-  ASSERT_TRUE(my_streq(LIBC_NAMESPACE::getenv("FRANCE1"),
-                       static_cast<char *>(nullptr)));
+  ASSERT_TRUE(LIBC_NAMESPACE::getenv("") == nullptr);
+  ASSERT_TRUE(LIBC_NAMESPACE::getenv("=") == nullptr);
+  ASSERT_TRUE(LIBC_NAMESPACE::getenv("MISSING ENV VARIABLE") == nullptr);
+  ASSERT_FALSE(LIBC_NAMESPACE::getenv("PATH") == nullptr);
+  ASSERT_EQ(LIBC_NAMESPACE::strcmp(LIBC_NAMESPACE::getenv("FRANCE"), "Paris"),
+            0);
+  ASSERT_NE(LIBC_NAMESPACE::strcmp(LIBC_NAMESPACE::getenv("FRANCE"), "Berlin"),
+            0);
+  ASSERT_EQ(LIBC_NAMESPACE::strcmp(LIBC_NAMESPACE::getenv("GERMANY"), "Berlin"),
+            0);
+  ASSERT_TRUE(LIBC_NAMESPACE::getenv("FRANC") == nullptr);
+  ASSERT_TRUE(LIBC_NAMESPACE::getenv("FRANCE1") == nullptr);
 
   return 0;
 }

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, thanks for the cleanup

Comment on lines 20 to 25
ASSERT_EQ(LIBC_NAMESPACE::strcmp(LIBC_NAMESPACE::getenv("FRANCE"), "Paris"),
0);
ASSERT_NE(LIBC_NAMESPACE::strcmp(LIBC_NAMESPACE::getenv("FRANCE"), "Berlin"),
0);
ASSERT_EQ(LIBC_NAMESPACE::strcmp(LIBC_NAMESPACE::getenv("GERMANY"), "Berlin"),
0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use inline_strcmp from https://github.com/llvm/llvm-project/blob/main/libc/src/string/memory_utils/inline_strcmp.h instead, so that this test won't depend on libc.src.string.strcmp entrypoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll make this update. I was away the past 2 weeks and am still unburying myself but will get to this and the other one shortly. Sorry for the delay!

… helper

Replace the custom my_streq() helper function with LIBC_NAMESPACE::strcmp(),
simplifying the test code and making it more consistent with other integration
tests in the codebase.

Changes:
- Removed 18-line my_streq() helper function
- Added #include "src/string/strcmp.h"
- Updated string comparisons:
  * Null checks: Direct comparison with nullptr
  * Equality checks: ASSERT_EQ(strcmp(...), 0)
  * Inequality checks: ASSERT_NE(strcmp(...), 0)
- Added libc.src.string.strcmp dependency in CMakeLists.txt

This reduces the test file from 49 to 31 lines while maintaining the same
test coverage. The strcmp-based approach is cleaner and aligns with the
pattern used in the environment_management branch tests (setenv_test,
unsetenv_test, putenv_test).

Testing: Verified that all test assertions pass with the new implementation.
@github-actions
Copy link

github-actions bot commented Nov 13, 2025

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

@kaladron kaladron merged commit 56eef98 into llvm:main Nov 13, 2025
20 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.

4 participants