Issue 1455 pre generate strings for months #1456
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
On reviewing the code I was focussed on I figured I could add a couple of slices and then simplify some the code around getting month name abbreviations. At this point I thought I'd get some input, in case you want to go a different way? Or, if you like this approach if you want to implement for all languages I could go ahead and do that. Either way, it seemed worth getting some feedback before going any further.
There were finctions where we do not need to convert to rune and then to string. Removing these casts prevents things escaping to the heap
xuri
added
the
size/S
|
@xuri I spotted some commented out code I'd left behind. I tidied this up in the most recent push. Hopefully this is looking good now, let me know what you think |
Codecov Report
@@ Coverage Diff @@
## master #1456 +/- ##
=======================================
Coverage 98.61% 98.61%
=======================================
Files 31 31
Lines 24015 24026 +11
=======================================
+ Hits 23682 23693 +11
Misses 220 220
Partials 113 113
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
xuri
approved these changes
Jan 25, 2023
xuri
pushed a commit
to JDavidVR/excelize
that referenced
this pull request
Jul 11, 2023
…ormat (qax-os#1456) - Reducing string concatenation and string conversion between rune string data types Co-authored-by: Nathan Davies <[email protected]>
jenbonzhang
pushed a commit
to jenbonzhang/excelize
that referenced
this pull request
Oct 22, 2023
…ormat (qax-os#1456) - Reducing string concatenation and string conversion between rune string data types Co-authored-by: Nathan Davies <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On reviewing the code I was focussed on I figured I could add a couple of slices and then simplify some the code around getting month name abbreviations. At this point I thought I'd get some input, in case you want to go a different way? Or, if you like this approach if you want to implement for all languages I could go ahead and do that.
I have also removed some of the unnecessary
string([]rune(...casts that were in place. Doing this prevents these tings from escaping to the heapEither way, it seemed worth getting some feedback before going any further.
Description
As an example of the proposed change I've introduced a couple of month abbreviation slices and shown how these could be used to simplify their corresponding functions. The existing tests already cover this code and continue to pass.
Related Issue
issue-1455
Motivation and Context
This is an attempt to reduce the memory needed by these language functions that have string concatenation involved.
How Has This Been Tested
The existing tests cover this code and they continue to pass. I do not think extra tests are needed here. Since adding these changes I can see, via
go build -gcflags="-m -l", that there is less that escapes to the heap.Types of changes
Checklist