expr: fix builtin functions precedence #8162
Conversation
|
GNU testsuite comparison: |
| new_ucmd!() | ||
| .args(&["index", "abcd", "c", "!=", "2"]) | ||
| .succeeds() | ||
| .stdout_only("1\n"); | ||
|
|
||
| new_ucmd!() | ||
| .args(&["index", "abcd", "c", "=", "2"]) | ||
| .fails_with_code(1) | ||
| .stdout_only("0\n"); |
There was a problem hiding this comment.
What's the reason you deviate from the pattern used for substr and length, where the commands with = are expected to succeed and the commands with != are expected to fail?
There was a problem hiding this comment.
I haven't been following this, but it probably makes sense to test all combinations right?
There was a problem hiding this comment.
The reason for that was that I wrote tests before fixing the bug and in case of index the first test case, for &["index", "abcd", "c", "!=", "3"] passed even before the fix, so I changed it to fail.
Now, the second test case for index (&["index", "abcd", "c", "=", "2"]) would pass also without the fix and match is missing such second test case.
I guess it's a good idea to make all cases consistent, like so:
#[test]
fn test_builtin_functions_precedence() {
new_ucmd!()
.args(&["substr", "ab cd", "3", "1", "!=", " "])
.fails_with_code(1)
.stdout_only("0\n");
new_ucmd!()
.args(&["substr", "ab cd", "3", "1", "=", " "])
.succeeds()
.stdout_only("1\n");
new_ucmd!()
.args(&["length", "abcd", "!=", "4"])
.fails_with_code(1)
.stdout_only("0\n");
new_ucmd!()
.args(&["length", "abcd", "=", "4"])
.succeeds()
.stdout_only("1\n");
new_ucmd!()
.args(&["index", "abcd", "c", "!=", "3"])
.fails_with_code(1)
.stdout_only("0\n");
new_ucmd!()
.args(&["index", "abcd", "c", "=", "3"])
.succeeds()
.stdout_only("1\n");
new_ucmd!()
.args(&["match", "abcd", "ab\\(.*\\)", "!=", "cd"])
.fails_with_code(1)
.stdout_only("0\n");
new_ucmd!()
.args(&["match", "abcd", "ab\\(.*\\)", "=", "cd"])
.succeeds()
.stdout_only("1\n");
}
Should I apply those changes to the PR?
There was a problem hiding this comment.
Changes applied, rebased on the latest main branch, and verified that for each expr builtin function (match, substr, index, length) at least one of the test cases fails without the fix.
|
GNU testsuite comparison: |
|
Thanks! |
Follow up to #8158
The issue #8063 found in
expr substrwas also present inindex,length,match. This PR applies the same fix for those remaining builtin expr functions.