Skip to content

Modernize SIP string handling#1645

Draft
paulomorgado wants to merge 4 commits into
sipsorcery-org:masterfrom
paulomorgado:strings-4
Draft

Modernize SIP string handling#1645
paulomorgado wants to merge 4 commits into
sipsorcery-org:masterfrom
paulomorgado:strings-4

Conversation

@paulomorgado
Copy link
Copy Markdown
Contributor

Update SIP and user-agent code to use clearer interpolation, span/string helpers, and explicit string comparison semantics. Add Polyfill support required by the new string APIs and remove the duplicate KeyValuePair Deconstruct extension.

Split of #1639

Comment thread src/SIPSorcery/core/SIP/SIPConstants.cs Outdated
@paulomorgado paulomorgado force-pushed the strings-4 branch 2 times, most recently from 98d5b86 to 4d3b642 Compare May 29, 2026 16:37
@sipsorcery
Copy link
Copy Markdown
Member

This PR did introduce a few new bugs in the SIP logic. One would have broken any SIP request that used the Route or Record-Route headers. The unit tests that show the bugs have been committed to master and if you rebase this PR the new tests will fail.

Overall the improvements do generally modernise the code but the size of PRs like this is unmanageable. The concerns need to be done separately, i.e. if it's jsut to update string formatting that should be one PR. Any functional changes should be done in a different PR. It tkaes mroe work to review PRs of this size than it does to write them.

paulomorgado and others added 4 commits May 29, 2026 23:16
Update SIP and user-agent code to use clearer interpolation, span/string helpers, and explicit string comparison semantics. Add Polyfill support required by the new string APIs and remove the duplicate KeyValuePair Deconstruct extension.
Reformat and clarify XML documentation comments for readability. Refactor SIPExtensionHeaders.ParseSIPExtensions to use a switch statement and HashSet for improved clarity and efficiency when parsing known and unknown SIP extensions. Apply minor code style improvements throughout.
Updated three ToString() methods in SIPSorcery.SIP (SIPHeader.cs) to use the override keyword, ensuring proper method overriding and improved polymorphic behavior.
@paulomorgado
Copy link
Copy Markdown
Contributor Author

The issue was the ToString() method declared as public new String() instead of public override String().

I had already run into 2 of these that were covered by tests.

And now I found there are other 6. Why?

@sipsorcery
Copy link
Copy Markdown
Member

And now I found there are other 6. Why?

Just the way I did it at the time, it was ~20 years ago! It doesn't matter so much as to the "why" as to the fact that the current behaviour works. Adding breaking "optimisations" would be a BIG problem for library users.

@paulomorgado paulomorgado marked this pull request as draft May 30, 2026 20:53
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.

2 participants