Skip to content
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

Removes a smattering of, apparent, dead code #10091

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

telser
Copy link

@telser telser commented Jun 9, 2024

Done during Zurihac:

Using weeder to find unused definitions. The roots, per weeder, should be all of the executables built from the top level directory. So all of the items removed here would appear to not be used in the execution or test paths of the codebase. There are a great many more items flagged by weeder, but this was an attempt to be relatively conservative in the removal.

This focuses the removal on internal facing items only. Which limits the gain, but also the scope.

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

@telser telser force-pushed the telser/remove-smattering-deadcode branch 4 times, most recently from f009c2e to f06457e Compare June 10, 2024 11:17
@telser telser marked this pull request as ready for review June 10, 2024 12:07
@andreabedini andreabedini force-pushed the telser/remove-smattering-deadcode branch from f06457e to 0659052 Compare June 11, 2024 10:53
@andreabedini
Copy link
Collaborator

Rebasing to include cb1b60f

@telser
Copy link
Author

telser commented Jun 13, 2024

Thanks @andreabedini!

@grayjay
Copy link
Collaborator

grayjay commented Jun 13, 2024

Do you happen to still have the changes to cabal-install-solver from the draft? They looked like safe changes.

@telser
Copy link
Author

telser commented Jun 13, 2024

Do you happen to still have the changes to cabal-install-solver from the draft? They looked like safe changes.

I do, along with some more that should be safe for cabal-install. The idea was to open that as a follow-on to this.

@andreabedini
Copy link
Collaborator

andreabedini commented Jun 17, 2024

I do, along with some more that should be safe for cabal-install. The idea was to open that as a follow-on to this.

What caused the difference between f009c2e and f06457e? Did you decide to start with some more conservative filter?

@Mikolaj
Copy link
Member

Mikolaj commented Aug 22, 2024

@telser: ping! Any final touches from you before we start the review? Any final comments (or answers to comments)?

@telser
Copy link
Author

telser commented Aug 23, 2024

I do, along with some more that should be safe for cabal-install. The idea was to open that as a follow-on to this.

What caused the difference between f009c2e and f06457e? Did you decide to start with some more conservative filter?

Being more conservative at the beginning was a suggestion of someone at Zurihac, though now I'm struggling to recall exactly who that was. Don't want to assign credit/blame without being sure.

@telser: ping! Any final touches from you before we start the review? Any final comments (or answers to comments)?

Thanks bringing this back to mind @Mikolaj! Per the above, I'm happy to go with this as-is or if the consensus to to be more aggressive upfront then amending to be somewhere between what is open and the draft is completely fine by me.

@@ -44,7 +43,6 @@ versionTests =
, tp "normaliseVersionRange involutive" prop_normalise_inv
, tp "normaliseVersionRange equivalent" prop_normalise_equiv
, tp "normaliseVersionRange caretequiv" prop_normalise_caret_equiv
, tp "normaliseVersionRange model" prop_normalise_model
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this test being removed?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, this was removed because the underlying code was going away as was part of the larger f009c2e originally. I can clean this up in either direction by reinstating the test or by adding to the removals. Any thoughts on the direction?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, to get this PR merged ASAP, before it bit-rots, let's re-add the test and leave the extra removal for future consideration?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I concur

Copy link
Author

Choose a reason for hiding this comment

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

I've restored this, thanks all!

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Overall looks good. Sorry it took so long...

I have only one question above.

@gbaz
Copy link
Collaborator

gbaz commented Oct 10, 2024

bump on the above question -- i would like to approve and merge but this should be resolved.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 5, 2024

Ping, ping?

@telser
Copy link
Author

telser commented Dec 8, 2024

bump on the above question -- i would like to approve and merge but this should be resolved.

Ping, ping?

Thank you for the ping and apologies for taking so long to get back to this! I believe I've answered the question above, but need some guidance on direction that others are comfortable with.

@Kleidukos
Copy link
Member

@telser Do you have all the guidance you need at this point? :)

Using weeder to find unused definitions. There are a great many more,
but this was an attempt to be relatively conservative in the removal.
@telser telser force-pushed the telser/remove-smattering-deadcode branch from 0659052 to 68153bb Compare January 2, 2025 00:49
@telser
Copy link
Author

telser commented Jan 2, 2025

@telser Do you have all the guidance you need at this point? :)

I believe so, thanks!

Have restored the above mentioned test and rebased against master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants