ardovm commented on code in PR #486:
URL: https://github.com/apache/openoffice/pull/486#discussion_r3455022429
##########
main/sal/qa/rtl/ostring/rtl_str.cxx:
##########
@@ -37,13 +37,32 @@ namespace rtl_str
TEST_F(compare, compare_000)
{
- rtl_str_compare( NULL, NULL);
+ // The former test passed (NULL, NULL). rtl_str_compare documents that
Review Comment:
"The former test" is IMHO unclear.
We could better phrase it as "A former version of this test"
##########
main/sal/qa/rtl/ostring/rtl_str.cxx:
##########
@@ -37,13 +37,32 @@ namespace rtl_str
TEST_F(compare, compare_000)
{
- rtl_str_compare( NULL, NULL);
+ // The former test passed (NULL, NULL). rtl_str_compare documents that
+ // both strings must be null-terminated, so NULL is undefined behaviour
+ // (the implementation unconditionally dereferences both pointers).
+ // Instead verify the ordering-sign contract, which had no coverage:
+ // a value < 0 / > 0 depending on which string is "less".
+ rtl::OString aStr1 = "abc";
+ rtl::OString aStr2 = "abd";
+
+ ASSERT_TRUE(rtl_str_compare( aStr1.getStr(), aStr2.getStr()) < 0)
+ << "\"abc\" must compare less than \"abd\".";
+ ASSERT_TRUE(rtl_str_compare( aStr2.getStr(), aStr1.getStr()) > 0)
+ << "\"abd\" must compare greater than \"abc\".";
}
TEST_F(compare, compare_000_1)
{
+ // The former test passed (validStr, NULL) which is undefined
behaviour.
+ // Verify the empty-string boundary instead (the valid analogue of an
Review Comment:
Ditto: "A former version of this test..."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
