-
Notifications
You must be signed in to change notification settings - Fork 158
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
Tidy up code and eliminate some allocations #378
Conversation
iamcarbon
commented
Jan 27, 2024
- Eliminate a possible virtual call in ByteTrieNode
- Updates ShouldAcceptRiffIdentifier to accept utf-8 bytes (eliminating a string allocation)
- Eliminates an allocation in GifReader
- Updates a few classes to use primary constructors
- Replaces some || statements with pattern matching
- Avoids an extra dictionary lookup
- Organizes using statements
- Fixes a few files with mixed line endings
@drewnoakes Ready for review / comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Good to go as is. Left a few thoughts if you want them. Can merge when you're happy.
MetadataExtractor/Formats/Exif/makernotes/FujifilmMakernoteDescriptor.cs
Show resolved
Hide resolved
@@ -482,34 +482,21 @@ public OlympusMakernoteDescriptor(OlympusMakernoteDirectory directory) | |||
if (Directory.GetObject(OlympusMakernoteDirectory.TagWbMode) is not short[] values) | |||
return null; | |||
|
|||
switch ($"{values[0]} {values[1]}".Trim()) | |||
return $"{values[0]} {values[1]}".Trim() switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a switch on a tuple, to avoid the string allocation and hopefully enable a jump table.
Eg
return $"{values[0]} {values[1]}".Trim() switch | |
return (values[0], values[1]) switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a dependency to ValueTuple, and updated.
@drewnoakes Ready to merge if everything looks good to you. I'll hit the rest in another PR. |
Thanks again! |