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++][NFC] Make AssertionInfoMatcher::CheckMessageMatches Stricter #77721

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

hawkinsw
Copy link
Contributor

Rather than allow for a message to be considered a match for the actual assertion if it is anywhere in the assertion text, make sure that the expected and the actual assertion are identical.

Addresses #77701

Rather than allow for a message to be considered a match for the actual
assertion if it is anywhere in the assertion text, make sure that the
expected and the actual assertion are identical.
@hawkinsw hawkinsw requested a review from a team as a code owner January 11, 2024 03:41
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 11, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 11, 2024

@llvm/pr-subscribers-libcxx

Author: Will Hawkins (hawkinsw)

Changes

Rather than allow for a message to be considered a match for the actual assertion if it is anywhere in the assertion text, make sure that the expected and the actual assertion are identical.

Addresses #77701


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

1 Files Affected:

  • (modified) libcxx/test/support/check_assertion.h (+1-2)
diff --git a/libcxx/test/support/check_assertion.h b/libcxx/test/support/check_assertion.h
index 98dd95b11556e6..01e296d9138d0c 100644
--- a/libcxx/test/support/check_assertion.h
+++ b/libcxx/test/support/check_assertion.h
@@ -89,8 +89,7 @@ struct AssertionInfoMatcher {
     std::size_t found_at = got_msg.find(msg_);
     if (found_at == std::string_view::npos)
       return false;
-    // Allow any match
-    return true;
+    return found_at == 0 && got_msg.size() == msg_.size();
   }
 private:
   bool is_empty_;

@hawkinsw
Copy link
Contributor Author

Please feel free to ignore this change if it is unwanted. The tests that I ran locally all seem to pass with the stricter checking. I'll wait to see what it looks like on a full run of CI.

…tricter

Fix a few locations where matches were not precise.
@llvmbot llvmbot added the cmake Build system in general and CMake in particular label Jan 11, 2024
Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

In general I like this patch. However there seems to be unrelated changes.

llvm/cmake/modules/LLVMExternalProjectUtils.cmake Outdated Show resolved Hide resolved
libcxx/test/support/check_assertion.h Show resolved Hide resolved
@mordante mordante self-assigned this Jan 11, 2024
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

I am fine with this change if it passes CI (and without the unrelated changes). I thought we took advantage of this "hand wavyness" in more places, but if we don't and the CI is happy, I think this is an improvement.

@hawkinsw
Copy link
Contributor Author

Asking for another review from @mordante just out of caution!

@hawkinsw
Copy link
Contributor Author

Pinging @var-const given his work on #77883. I will wait for his "go" before moving forward.

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

Thanks LGTM! Let's ship it.

@hawkinsw hawkinsw merged commit 882b4fc into llvm:main Jan 12, 2024
43 checks passed
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…llvm#77721)

Rather than allow for a message to be considered a match for the actual
assertion if it is anywhere in the assertion text, make sure that the
expected and the actual assertion are identical.

Addresses llvm#77701
@hawkinsw hawkinsw deleted the assertion_matcher branch January 31, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants