-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc][stdlib] Simplify getenv_test by using strcmp instead of custom helper #163055
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
Conversation
|
@llvm/pr-subscribers-libc Author: Jeff Bailey (kaladron) ChangesReplace 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:
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:
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;
}
|
michaelrj-google
left a comment
There was a problem hiding this 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
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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:
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.