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

feat(stdlib): strncpy consistency and add strlcpy #6204

Merged
merged 4 commits into from May 22, 2024

Conversation

liamHowatt
Copy link
Collaborator

Description of the feature or fix

Following up with the discussion about strncpy in #5927

Make lv_strncpy consistent with the standard spec.

The standard spec of strncpy isn't necessarily better than the one that's implemented in LVGL lv_string_builtin.c. The concern is that the behavior of lv_strncpy changes depending on the configured LV_USE_STDLIB_STRING. We wrap the libc strncpy with the modified behavior to match the unique builtin behavior, however we don't wrap the rt-thread implementation and the rt-thread implementation matches the standard spec, i.e. the libc behavior.

I updated the uses of lv_strncpy where it's called to ensure a null terminator is added where it's expected.

See the manpage for strncpy.

Notes

@kisvegabor
Copy link
Member

I feel like our behavior for lv_strncpy is better 🤣 I really can't understand why the standard is filling all the remaining place with zeros and doesn't ensure null termination.

So I would rather make RT-Thread's (ad the others') implementation like ours because this way we need to make this only a few places instead of in the whole code base where contributors might forget adding the closing zero.

@XuNeo
Copy link
Collaborator

XuNeo commented May 9, 2024

I feel like our behavior for lv_strncpy is bette

You are looking for strlcpy. Let's use lv_strlcpy then. The standard is fixed, we cannot do much about it now.

I didn't realize we could use strlcpy instead until recently :(

@kisvegabor
Copy link
Member

I didn't know strlcpy either, but it's great! I agree to use lv_strlcpy. @liamHowatt if you also agree could you update this PR accordingly?

@liamHowatt
Copy link
Collaborator Author

I agree. I'll add strlcpy and use it in most places strncpy is.

@liamHowatt
Copy link
Collaborator Author

Updated.

So it looks like the strlcpy is a BSD thing. You need libbsd installed. So I just implemented the clib one in terms of strlen and memcpy so people using clib string don't need libbsd.

FYI strlcpy returns the length of src. Something to note.

XuNeo
XuNeo previously approved these changes May 20, 2024
src/others/file_explorer/lv_file_explorer.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@XuNeo XuNeo left a comment

Choose a reason for hiding this comment

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

Thank you!

@liamHowatt liamHowatt changed the title feat(stdlib): strncpy consistency feat(stdlib): strncpy consistency and add strlcpy May 21, 2024
Copy link
Member

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

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

Thank you!

I'm not sure we need to keep lv_strncpy but let's leave it for backward compatibility.

@kisvegabor kisvegabor merged commit 3301686 into lvgl:master May 22, 2024
19 checks passed
@kdschlosser
Copy link
Contributor

kdschlosser commented May 22, 2024

@kisvegabor

I think this is the one that broke the MicroPython CI.

The rest relies on loading fonts from the file system and this change specifically alters the file system drivers...

@liamHowatt
Copy link
Collaborator Author

That CI was run before this got merged.

@kdschlosser
Copy link
Contributor

There are only 2 things that got merged in that time frame and this is the only one that would effect the micropython CI

Maybe something got changed over in the micropython binding?

@kdschlosser
Copy link
Contributor

nothing changed over there either. Gonna have to turn on debugging for the test to see what is happening.

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.

None yet

4 participants