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: strchr tests: add test group for ASCII compare #195

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

maska989
Copy link
Contributor

@maska989 maska989 commented Apr 26, 2023

Description

Added strcspn and strspn test cases

Motivation and Context

JIRA: CI-232

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

  • Already covered by automatic testing.
  • New test added: (add PR link here).
  • Tested by hand on: (list targets here).

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

  • This PR needs additional PRs to work (list the PRs, preferably in merge-order).
  • I will merge this PR by myself when appropriate.

@maska989 maska989 marked this pull request as draft April 26, 2023 07:43
libc/string_lenchr.c Outdated Show resolved Hide resolved
@maska989 maska989 marked this pull request as ready for review May 23, 2023 07:27
Copy link
Member

@mateusz-bloch mateusz-bloch left a comment

Choose a reason for hiding this comment

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

Thanks for this tests. I've pointed out some small nitpicks.

Also I would consider renaming the commit to more closely match the changes, and change beginning of the file:

 * TESTED:
 * 	- strlen()
 * 	- strnlen()
 *     -  strcspn()
 *     -  strspn()
 * 	- strchr()
 * 	- strrchr()
 * 	- memchr()

EDIT: You can also tidy up included files.
Aren't these all you need in this test ?:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <limits.h>
#include <unity_fixture.h>

libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
@maska989 maska989 force-pushed the Maska989/testStrcspn branch 2 times, most recently from 3aa890b to 2d941b2 Compare May 25, 2023 13:08
mateusz-bloch
mateusz-bloch previously approved these changes Jun 2, 2023
Copy link
Member

@mateusz-bloch mateusz-bloch left a comment

Choose a reason for hiding this comment

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

LGTM, don't forget to organize your commits before merge, also i would wait for @damianloew to approve

Copy link
Contributor

@damianloew damianloew left a comment

Choose a reason for hiding this comment

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

@mateusz-bloch thanks for previous reviews and @maska989 for much cleaner code, a few things more to add/change.

libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
@maska989 maska989 force-pushed the Maska989/testStrcspn branch 2 times, most recently from 73a7370 to a1d3464 Compare June 15, 2023 08:02
@maska989 maska989 requested a review from damianloew June 16, 2023 07:20
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
Copy link
Contributor

@damianloew damianloew left a comment

Choose a reason for hiding this comment

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

A few things more to improve, please look at the suggestions above.

@maska989 maska989 force-pushed the Maska989/testStrcspn branch 2 times, most recently from 79ef939 to 060cdd7 Compare July 6, 2023 12:35
@maska989 maska989 requested a review from damianloew July 10, 2023 08:52
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
Comment on lines 269 to 303
TEST(string_spn, empty_output)
{
TEST_ASSERT_EQUAL_INT(0, strcspn("", "abc"));
TEST_ASSERT_EQUAL_INT(0, strspn("", "abc"));

TEST_ASSERT_EQUAL_INT(0, strcspn("", ""));
TEST_ASSERT_EQUAL_INT(0, strspn("", ""));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When naming it empty_output do you mean that 0 should be returned? If yes you can also add there strspn("abc", "") and strcspn("abc", "bc")

Copy link
Contributor

@damianloew damianloew Jul 10, 2023

Choose a reason for hiding this comment

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

Eventually it could be the case where empty arguments are checked, and then strcspn("abc", "") and strcspn("", "abc") should be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strcspn("", "abc")

This element is present on commented lines.

libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string_lenchr.c Outdated Show resolved Hide resolved
@maska989 maska989 force-pushed the Maska989/testStrcspn branch 3 times, most recently from d1d091b to 98fea8a Compare July 13, 2023 13:09
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
libc/string/string_lenchr.c Outdated Show resolved Hide resolved
char *reversStr,
*testStr;

testStr = testdata_createCharStr(BUFF_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what was the intention, but if you want to have all ascii chars you have to set size to 129.

Please edit the description of this function in testdata.h:

/* 
 * for (size >= ALL_CHARS_STRING_SIZE) returns string filled with all possible chars.
 * for (size < ALL_CHARS_STRING_SIZE) returns string with the following content: {1,1,2,...(size-2),0}.
 * The returned pointer has to be freed after use!
 */
extern char *testdata_createCharStr(int size);

Would it be more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is done in #199

@damianloew damianloew merged commit 18a8803 into master Jul 18, 2023
24 checks passed
@damianloew damianloew deleted the Maska989/testStrcspn branch July 18, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants