Skip to content

Conversation

@nojaf
Copy link
Contributor

@nojaf nojaf commented Jan 11, 2023

In light of #14567, I believe to have found some code that is unused.

These are top-level functions that are unused in the file they are defined and are not part of the signature.

In some cases, I did update the signature file. (For example in Result.toSeq in FSharp.Core, because the value was missing in src/FSharp.Core/result.fsi it was considered unused).

And in sformat.fs I did shuffle some functions a bit because depending on the COMPILER define value the code was considered to be unused or not. This file is used both in FCS and FSharp.Core.

@nojaf nojaf requested a review from a team as a code owner January 11, 2023 15:52
Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

I love this change, but I want to understand whether the reordering of the baseline file is benign.
Also there is too much churn in that file to see whether something interesting was removed.

So if we can hold off on merging for a short while.

@T-Gro
Copy link
Member

T-Gro commented Jan 11, 2023

I want to nominate this for PR of the year.

@KevinRansom
Copy link
Contributor

I'm gonna do the baselines for this differently, so the big delta here is fine for now.

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Now I think I understand what is going on with the baseline.

@nojaf
Copy link
Contributor Author

nojaf commented Jan 13, 2023

@T-Gro I've addressed all the remarks. This one is good to go.

@T-Gro T-Gro merged commit cb106cf into dotnet:main Jan 13, 2023
@nojaf nojaf deleted the remove-unused-code branch January 13, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants