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

[Support][NFC] Add test documenting that empty Regex pattern matches nothing. #83849

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

martinboehme
Copy link
Contributor

I was wondering about this when I recently used Regex, and I thought it would
be nice to have a test documenting this behavior.

…s nothing.

I was wondering about this when I recently used `Regex`, and I thought it would
be nice to have a test documenting this behavior.
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 4, 2024

@llvm/pr-subscribers-llvm-support

Author: None (martinboehme)

Changes

I was wondering about this when I recently used Regex, and I thought it would
be nice to have a test documenting this behavior.


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

1 Files Affected:

  • (modified) llvm/unittests/Support/RegexTest.cpp (+8)
diff --git a/llvm/unittests/Support/RegexTest.cpp b/llvm/unittests/Support/RegexTest.cpp
index 09f674bb209c07..c6ac42591d1fc7 100644
--- a/llvm/unittests/Support/RegexTest.cpp
+++ b/llvm/unittests/Support/RegexTest.cpp
@@ -60,6 +60,14 @@ TEST_F(RegexTest, Basics) {
   EXPECT_TRUE(r5.match(String));
 }
 
+TEST_F(RegexTest, EmptyPattern) {
+  // The empty pattern doesn't match anything -- not even the empty string.
+  // (This is different from some other regex implementations.)
+  Regex r("");
+  EXPECT_FALSE(r.match("123"));
+  EXPECT_FALSE(r.match(""));
+}
+
 TEST_F(RegexTest, Backreferences) {
   Regex r1("([a-z]+)_\\1");
   SmallVector<StringRef, 4> Matches;

@dwblaikie
Copy link
Collaborator

if this is different from other regex implementations, could we make it do something more like other implementations so as to be less surprising? (is this a feature of the underlying regex library we use - or something we added on top?)

Couldn't find clear documentation in the libc regex (at a glance/quick googling) that this is all based on - but doesn't look like we special case it in any way, so probably a feature of libc's regex...

@martinboehme
Copy link
Contributor Author

CI failure is a failing test in Flang that is unrelated.

@martinboehme
Copy link
Contributor Author

if this is different from other regex implementations, could we make it do something more like other implementations so as to be less surprising? (is this a feature of the underlying regex library we use - or something we added on top?)

I don't see anything in Regex.cpp that special-cases this, so I assume it's a feature of the libc regex code.

Couldn't find clear documentation in the libc regex (at a glance/quick googling) that this is all based on - but doesn't look like we special case it in any way, so probably a feature of libc's regex...

Agree.

I have to admit I'm not sure why we do this, or why the libc regex does it -- but I have to assume that some user of Regex relies on this behavior (knowingly or unknowingly), so I'm hesitant to change it. I did, however, at least want to get a test in documenting the behavior.

@martinboehme martinboehme merged commit d1d2932 into llvm:main Mar 5, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants