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

Fix the characteristic that the trim() and trim_end() functions do not remove the \0 character. #620

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xb284524239
Copy link

@xb284524239 xb284524239 commented Dec 30, 2024

fix #618

Hello @Enet4 ,

I have standardized the codebase by uniformly substituting the trim() and trim_end() functions with the trim_end_matches([' ', '\u{0}']) function, which was replicated from the to_str() function within the primitive.rs file.

I am rather uncertain as to the rationale behind the concurrent existence of the trim() and trim_end() and trim_end_matches([' ', '\u{0}']) functions in the previous implementation. Was this a deliberate design choice?

Moreover, I am pondering whether it might be more apt to utilize the trim_matches(|c: char| c == '\0' || c.is_whitespace()) function instead. However, I remain indecisive on this matter.

…_matches([' ', '\u{0}']) function. Because the trim() / trim_end() function in Rust does not remove the \0 character.
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

I have standardized the codebase by uniformly substituting the trim() and trim_end() functions with the trim_end_matches([' ', '\u{0}']) function, which was replicated from the to_str() function within the primitive.rs file.

I am rather uncertain as to the rationale behind the concurrent existence of the trim() and trim_end() and trim_end_matches([' ', '\u{0}']) functions in the previous implementation. Was this a deliberate design choice?

I believe that the occurrences of both trim* and trim_end* are indeed deliberate. This is because some values admit both leading and trailing whitespace. https://dicom.nema.org/medical/dicom/2024d/output/chtml/part05/sect_6.2.html In the cases where trim() was called, please retain the begin trimming.

Moreover, I am pondering whether it might be more apt to utilize the trim_matches(|c: char| c == '\0' || c.is_whitespace()) function instead. However, I remain indecisive on this matter.

I would stick to checking specifically for spaces, as that is what's listed by the standard. is_whitespace may cover more cases of whitespace than necessary and/or intended.

@Enet4 Enet4 added enhancement A-lib Area: library C-core Crate: dicom-core labels Jan 1, 2025
@xb284524239 xb284524239 marked this pull request as draft January 1, 2025 23:31
@xb284524239
Copy link
Author

Hello @Enet4 ,
The changes to the calls of trim() and trim_end() have been restored, and only the matching for the \0 character has been added.

@xb284524239 xb284524239 marked this pull request as ready for review January 2, 2025 02:32
@Enet4 Enet4 self-requested a review January 11, 2025 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lib Area: library C-core Crate: dicom-core enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It crashed when I using obj.element(tags::WINDOW_WIDTH)?.to_multi_float64()?
2 participants