Skip to content

Conversation

@baronfel
Copy link
Member

@baronfel baronfel commented Jan 13, 2019

This is the start of fixing #6084 according to the suggestions in that thread about chunking the ResizeArray. I need to do testing locally as well but I'll have to boot up my windows machine and get a dev environment set up so it could take a minute.

I'm mostly putting this out to verify the approach/get CI validation as I continue working to clean it up and get to a good final solution.

TODO:

  • revert whitespace changes (thanks Rider :D )
  • double-check all the index math. I tested briefly in FSI but I'm sure I missed something.

@cartermp cartermp changed the title Tcsymbolusedata cleanup per #6084 [WIP] Tcsymbolusedata cleanup per #6084 Jan 13, 2019
@baronfel baronfel force-pushed the tcsymbolusedata-cleanup branch 2 times, most recently from 031f1a8 to 5b96683 Compare January 13, 2019 18:42
Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes requested above. You don't need to add specific tests for this

@dsyme
Copy link
Contributor

dsyme commented Jan 14, 2019

I checked the index logic, it's fine, suggested one improvement (but check it please! :) )

@baronfel baronfel force-pushed the tcsymbolusedata-cleanup branch from 5b96683 to ed22216 Compare January 14, 2019 16:04
@baronfel
Copy link
Member Author

Ok, I made the suggested changes, including special-casing zero-length ResizeArrays.

@cartermp
Copy link
Contributor

cartermp commented Jan 15, 2019

@baronfel Is this still WIP? (note: I changed the title earlier for when it still was)

@baronfel baronfel changed the title [WIP] Tcsymbolusedata cleanup per #6084 TcSymbolUseData cleanup per #6084 Jan 15, 2019
@baronfel
Copy link
Member Author

It's no longer WIP pending @dsyme accepting the changes made from his reviews.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the change. Let's get a @TIHan review since he's recently also been looking at array chunking for other things

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and a conservative approach.

@TIHan TIHan merged commit 7584974 into dotnet:master Jan 17, 2019
@TIHan
Copy link
Contributor

TIHan commented Jan 17, 2019

Thank you.

auduchinok added a commit to auduchinok/fsharp that referenced this pull request May 14, 2019
auduchinok added a commit to auduchinok/fsharp that referenced this pull request May 23, 2019
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Sep 28, 2019
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Mar 3, 2020
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Apr 27, 2020
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Jun 9, 2020
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Jul 20, 2020
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Jul 27, 2020
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Oct 22, 2020
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Oct 28, 2020
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Dec 11, 2020
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Mar 5, 2021
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Apr 2, 2021
auduchinok added a commit to auduchinok/fsharp that referenced this pull request May 17, 2021
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Jun 22, 2021
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Jul 12, 2021
auduchinok added a commit to auduchinok/fsharp that referenced this pull request Jul 19, 2021
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.

6 participants