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

Data.Graph.SCC: store mutually reachable vertices in a non-empty list #953

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

Bodigrim
Copy link
Contributor

As discussed in #952.

CC @meooow25

@Bodigrim
Copy link
Contributor Author

Bodigrim commented Jun 27, 2023

GHC 8.0 build fails because of the lack of instance {Eq,Ord,Show,Read}1 NonEmpty. TBH I'd suggest just to drop GHC 8.0, I have not seen anyone using it for ages.

The rest of CI failures are because of the absense of instance Lift NonEmpty prior to GHC 8.8. That's annoying, but fixable. I'll take a closer look if the rest of the PR seems mergeable.

containers/src/Data/Graph.hs Outdated Show resolved Hide resolved
containers/src/Data/Graph.hs Outdated Show resolved Hide resolved
containers/src/Data/Graph.hs Outdated Show resolved Hide resolved
containers/src/Data/Graph.hs Outdated Show resolved Hide resolved
containers/src/Data/Graph.hs Outdated Show resolved Hide resolved
@treeowl
Copy link
Contributor

treeowl commented Jun 28, 2023

There seem to be two issues remaining:

  1. Constructor name
  2. CI failure on 8.0

I don't really like the primed name, but I'm willing to go along with it if no one can come up with anything better. For the CI failure, I'm okay with dropping support for 8.0, but that will definitely need its own commit, and should probably be a separate PR merged before this one.

@Bodigrim
Copy link
Contributor Author

I don't really like the primed name, but I'm willing to go along with it if no one can come up with anything better.

NECyclicSCC? CyclicSCC1?

@treeowl
Copy link
Contributor

treeowl commented Jun 28, 2023

Could you please rebase this?

@treeowl
Copy link
Contributor

treeowl commented Jun 28, 2023

I don't really like the primed name, but I'm willing to go along with it if no one can come up with anything better.

NECyclicSCC? CyclicSCC1?

They're all horrible (and my own attempts were worse), but I think I dislike NECyclicSCC the least.

@Bodigrim
Copy link
Contributor Author

@treeowl all ready now. Shall I update changelog as well?

@treeowl
Copy link
Contributor

treeowl commented Jun 28, 2023

Yes, please!

@Bodigrim
Copy link
Contributor Author

Changelog updated.

@treeowl treeowl merged commit c8a24e8 into haskell:master Jun 28, 2023
9 checks passed
@treeowl
Copy link
Contributor

treeowl commented Jun 28, 2023

Thanks!!!

@Bodigrim Bodigrim deleted the scc branch June 28, 2023 23:18
deriving ( Eq -- ^ @since 0.5.9
, Show -- ^ @since 0.5.9
, Read -- ^ @since 0.5.9
)

-- | Partial pattern synonym for backward compatibility with @containers < 0.7@.
pattern CyclicSCC :: [vertex] -> SCC vertex
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the pattern synonym definition also need to be guarded by #ifdef __GLASGOW_HASKELL__ ?
I'm always confused by the portability story. Tangentially related, how do we even test this given CI only uses ghc?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it does. The portability story kind of sucks right now. The main goal as I see it is to make sure it won't be too big a pain to add support for a future Report-style Haskell (Purescript, sadly, is just a bit too far away to be a practical target for this). I think we should stop using __GLASGOW_HASKELL__ directly and instead use a few of our own macros. We should have a build job that pretends were not on GHC; getting that to do testing right will take a bit of fussing, but it's probably doable?

@Bodigrim Bodigrim mentioned this pull request Sep 27, 2023
treeowl added a commit to treeowl/containers that referenced this pull request Sep 28, 2023
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.

None yet

3 participants