Skip to content

Remove dead code #6188

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

Merged
merged 1 commit into from
Sep 8, 2019
Merged

Remove dead code #6188

merged 1 commit into from
Sep 8, 2019

Conversation

sgraf812
Copy link
Contributor

This would trip up the improved pattern match checker from !963.

@phadej
Copy link
Collaborator

phadej commented Aug 10, 2019

I'm on the fence. The code there is in the state to have roundtrip property for code like

if flag(some-condition)
  buildable: True
else
  buildable: False

which is ugly when pretty-printed as:

if flag(some-condition)
else
  buildable: False

That's because buildable: True is no-op, and there could be other a like.

One way, is to pretty-print that as

if !flag(some-condition)
  buildable: False

But that violates roundtrip property.


If the code is removed, then there should be an issue to think how to resolve (or not) the above.

@sgraf812
Copy link
Contributor Author

Well, I agree that you probably don't want to print empty if branches (IIUC), but currently it's just dead code, no? What I did was purely a refactoring. The line that actually did something has been commented out for 2 years. Unless there is some ugly templating mechanism involved in the build process that replaces the case (True, True) of by the line below, I don't think keeping the other branches makes sense at all.
If on the other hand that dead code makes actual sense, then we should probably think about the scrutinised expression...

@phadej
Copy link
Collaborator

phadej commented Aug 10, 2019

That dead code reminds me of an unsolved issue, but when it's gone there won't be anything reminding of that ugliness...

@sgraf812
Copy link
Contributor Author

sgraf812 commented Aug 10, 2019

Soo... When is that issue going to be resolved? Why does it mean we can't remove currently dead code? I want my patch to hit master within the next month, which is quite impossible when the code here stays this way. Of course, we could always attach a {-# OPTIONS_GHC -woverlapping-patterns #-} as a short term solution...

@DanielG
Copy link
Collaborator

DanielG commented Aug 10, 2019

How about replacing the dead code with a comment and an issue reference which everyone ca remember rather than storing this obscure information only in @phadej's brain?

@sgraf812
Copy link
Contributor Author

Is there any news on this, @phadej? Should I add a comment referencing this MR, expressing your plans to get back to it at some point? The deleted code is still in version control, so it's not like we could never recover it... Quite the contrary, leaving dead code seems like a code smell to me.

@sgraf812
Copy link
Contributor Author

I opened #6193 to track the issue here.

@sgraf812
Copy link
Contributor Author

I can't manually restart the failing jobs, short of force pushing with a different commit message. I don't know why the same two jobs keep running into the timeout.

@sgraf812
Copy link
Contributor Author

sgraf812 commented Sep 7, 2019

Could anyone merge this please? This is still holding back https://gitlab.haskell.org/ghc/ghc/merge_requests/963 to land on master... Apparently CI turned green thanks to some kind stranger restarting the failing jobs for me - thank you!

@DanielG
Copy link
Collaborator

DanielG commented Sep 8, 2019

I would love to but you still haven't addressed the comments I gave you on IRC i.e. adding a comment with a link to the issue.

@sgraf812
Copy link
Contributor Author

sgraf812 commented Sep 8, 2019

Ah, I thought you meant I should link from the commit message, which I did. Fair enough.

This would trip up the improved pattern match checker from [!963](https://gitlab.haskell.org/ghc/ghc/merge_requests/963).

The removed code is related to maintaining the round-trip property, so
should be re-introduced at some point. This is tracked in haskell#6193.
@phadej phadej merged commit 63331c9 into haskell:master Sep 8, 2019
@sgraf812 sgraf812 deleted the patch-1 branch September 8, 2019 12:02
@sgraf812
Copy link
Contributor Author

sgraf812 commented Sep 8, 2019

Thank you! :) What is the story for updating the GHC's Cabal submodule? Should I just submit a bump along with my MR? I suspect there will be some breakage...

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.

3 participants