Skip to content

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

Merged
Enet4 merged 2 commits intoEnet4:masterfrom
bowenxuuu:master
Jan 16, 2025
Merged

Fix the characteristic that the trim() and trim_end() functions do not remove the \0 character.#620
Enet4 merged 2 commits intoEnet4:masterfrom
bowenxuuu:master

Conversation

@bowenxuuu
Copy link
Copy Markdown
Contributor

@bowenxuuu bowenxuuu 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
Copy Markdown
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
@bowenxuuu bowenxuuu marked this pull request as draft January 1, 2025 23:31
@bowenxuuu
Copy link
Copy Markdown
Contributor 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.

@bowenxuuu bowenxuuu marked this pull request as ready for review January 2, 2025 02:32
@Enet4 Enet4 self-requested a review January 11, 2025 10:02
Copy link
Copy Markdown
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.

Should be OK. Anyone depending on the presence of nul characters in the output should be using the raw string method variants intead.

Thank you for your contribution @xb284524239. 👍

@Enet4 Enet4 merged commit 4afa668 into Enet4:master Jan 16, 2025
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