Change the definition of enum SIPExtensions to allow using as a bitfield #1323
Conversation
…eld. In class SIPHeader, members RequiredExtensions and SupportedExtensions can now become bitfields instead of lists of enums. This results in a smaller memory footprint. Modify static method ParseSIPExtensions accordingly, and eliminate all string concatenations by using a StringBuilder.
| if (!String.IsNullOrEmpty(extension)) | ||
| { | ||
| if (String.IsNullOrEmpty(extension) == false) | ||
| var trimmedExtension = extension.Trim().ToLower(); |
There was a problem hiding this comment.
This is potentially creating 2 strings and using current culture dependent operations.
I know the switch is convenient, but not when dealing with strings.
| } | ||
|
|
||
| foreach (string extension in extensions) | ||
| var extensions = extensionList.Split(','); |
There was a problem hiding this comment.
This allocates a string[] and several strings.
Consider using StringTokenizer
| break; | ||
|
|
||
| default: | ||
| unknownExtensionsStringBuilder.Append(extension.Trim()); |
|
You are totally right. I kept the same existing logic of the function, but I agree that the string manipulations are not done the best way, and I can go further if needed. |
|
The flag enum is a good improvement. Happy to merge. The change is highly unlikely to affect any library users. If there are further string optimisations planned it would be better to get them in now. |
|
Thanks for the review. That means that the visibility of the member could be restricted. I'll think about it. |
Change the definition of enum SIPExtensions to allow using as a bitfieldeld.
In class SIPHeader, members RequiredExtensions and SupportedExtensions can now become bitfields instead of lists of enums.
This results in a smaller memory footprint.
Modify static method ParseSIPExtensions accordingly, and eliminate all string concatenations by using a StringBuilder.
Important
Although this approch is more idiomatic to C#, and reduces the memory footprint of the library, it is important to note that this modifies the public API, as members SupportedExtensions are RequiredExtensions are public. Please let me know what you think about it.