On Fri, Jul 20, 2012 at 11:25:47PM +0200, Henning Thielemann wrote:

On Fri, 20 Jul 2012, Brent Yorgey wrote:

...

Use of GHC.Exts ---------------

At the request of a user, the 0.1.4.3 release switched from defining its own version of the standard 'build' function, to importing it from GHC.Exts. This allows GHC to do more optimization, resulting in reported speedups to uses of splitEvery, splitPlaces, and splitPlacesBlanks. However, this makes the library GHC-specific. If any reviewers think this is an issue I would be willing to go back to defining build by hand, or use CPP macros to select between build implementations based on the compiler.

You could provide two private modules with the same name in different directories, one that re-exports 'build' from GHC.Exts and one with a custom definition of 'build' for non-GHC compilers. Then set 'Hs-Source-Dirs' in Cabal according to the impl(ghc). No CPP needed, only Cabal. One could even think of a separate package for this purpose.

Ah, this is a good idea. I'd still like to hear from other reviewers whether they think it is worth the trouble. To what extent should the Haskell Platform try to be compiler-agnostic (even though it includes GHC)?

The only type extension you use, is GADTs, right? It looks like you use it for an Eq constraint in Delimiter/DelimSublist. That is, you actually need only ExistentialQuantification. Is it necessary?

You are right, actually, only ExistentialQuantification is necessary, as long as we also stop using GADT syntax. I didn't realize before that this syntax is accepted: {-# LANGUAGE ExistentialQuantification #-} data Delimiter a = DelimEltPred (a -> Bool) | Eq a => DelimSublist [a] I do agree that this is a bit weird, what's going on here is not exactly existential quantification. But in any case the ExistentialQuantification extension turns on this ability to embed class constraints in data constructors -- at least in GHC. I am happy to make this change, but I guess it again raises the issue GHC-specificity. As to whether there is a way to do this without using ExistentialQuantification, I don't see an obvious solution (though there probably is one). The issue is that we certainly don't want to require an (Eq a) constraint when using a predicate, but we need one when matching sublists. I'm open to suggestions. -Brent

On Sat, Jul 21, 2012 at 09:34:13AM +0200, Henning Thielemann wrote:

On Fri, 20 Jul 2012, Brent Yorgey wrote:

...

As to whether there is a way to do this without using ExistentialQuantification, I don't see an obvious solution (though there probably is one). The issue is that we certainly don't want to require an (Eq a) constraint when using a predicate, but we need one when matching sublists. I'm open to suggestions.

If you do not embed the Eq dictionary into DelimSublist then you have to add the Eq constraint to some functions. Is this a problem? In turn you would get a perfectly portable Haskell 98 module.

No, I do not want to do this. The reason is that you would no longer be able to do splitting over lists of elements with no Eq instance, even if you are using a predicate. For example, it is currently possible to do fs = [(+1), (subtract 7), (*6)] fs' = splitWhen (\f -> f 7 == 0) fs but this would be no longer possible with your patch. This is an admittedly contrived example, but on principle I don't want to unnecessarily restrict the API in this way. -Brent

An unrelated suggestion: you can give type signatures to the various functions which are synonyms of each other as a group and they will show up as a single item in the Haddocks.

Have you tried this a recent version of Haddock? I think this was only in the Haddock version that was released with GHC 7.2 (introduced due to a change in GHC). But it had issues with explicit export lists, so we now always expand such signatures. Cheers, Simon

On Sat, Jul 21, 2012 at 04:37:48PM +0200, Gábor Lehel wrote:

On Sat, Jul 21, 2012 at 2:13 PM, Simon Hengel wrote:

...

...

An unrelated suggestion: you can give type signatures to the various functions which are synonyms of each other as a group and they will show up as a single item in the Haddocks.

Have you tried this a recent version of Haddock? I think this was only in the Haddock version that was released with GHC 7.2 (introduced due to a change in GHC). But it had issues with explicit export lists, so we now always expand such signatures.

Cheers, Simon

It seems to work with Haddock 2.10 / GHC 7.4. I remember I initially tried it with whatever old version of Haddock I had installed and I was annoyed that it didn't work (it got split into two declarations with one of them missing documentation entirely -- quite suboptimal), but then following a suggestion I tried it again with GHC 7.4 and was happy that it seemed to have improved and gave the expected behaviour, and impressively even worked across module boundaries. I would be sad if it stopped working again. :)

Indeed, from the CHANGES file in the haddock distribution: Changes in version 2.9.3: * A type signature for multiple names generates one signature in the output Neato, I didn't know about this feature. I think grouping synonyms together like this is a good idea. -Brent

On 2012-07-21 02:34, Brent Yorgey wrote:

You are right, actually, only ExistentialQuantification is necessary, as long as we also stop using GADT syntax. I didn't realize before that this syntax is accepted:

{-# LANGUAGE ExistentialQuantification #-}

data Delimiter a = DelimEltPred (a -> Bool) | Eq a => DelimSublist [a]

Would the following type work? data Delimiter a = DelimEltPred (a -> Bool) | DelimSublistPred [a -> Bool] You can go from the current DelimSublist to DelimSublistPred with just `map (==)`. And is the distinction between the DelimEltPred and DelimSublistPred then still needed at all? Twan

On Sat, Jul 21, 2012 at 02:59:45PM +0200, Twan van Laarhoven wrote:

On 2012-07-21 14:30, Twan van Laarhoven wrote:

...

Would the following type work?

data Delimiter a = DelimEltPred (a -> Bool) | DelimSublistPred [a -> Bool]

You can go from the current DelimSublist to DelimSublistPred with just `map (==)`. And is the distinction between the DelimEltPred and DelimSublistPred then still needed at all?

I attached some code that uses the simplified Delimiter type.

Brilliant! Yes, this is a big improvement, and does indeed allow getting rid of the ExistentialQuantification (or GADTs) extension. I will make this change for sure, regardless of the outcome of the review process.

As an aside, the documentation for splitPlaces contains this unhelpful remark:

...

The behavior of splitPlaces ls xs when sum ls /= length xs can be inferred from the above examples and the fact that splitPlaces is total.

To me that reads like "the documentation of this function is left as an exercise to the reader". Perhaps say something like:

If the input list is longer than the total of the given lengths, then the remaining elements are dropped. If the list is shorter than the total of the given lengths, then the result may contain fewer chunks, and the last chunk may be shorter.

While `splitPlacesBlanks` could say something like:

If the input list is longer than the total of the given lengths, then the remaining elements are dropped. If the list is shorter than the total of the given lengths, then the last several chunk will be shorter or empty.

Ah, yes, I agree. I must have been in somewhat of a cheeky mood when I wrote that. I will improve the documentation, thanks for the suggestions. -Brent

On Mon, Jul 23, 2012 at 11:11:19AM +0200, Christian Maeder wrote:

Am 21.07.2012 14:30, schrieb Twan van Laarhoven:

...

Would the following type work?

data Delimiter a = DelimEltPred (a -> Bool) | DelimSublistPred [a -> Bool]

You can go from the current DelimSublist to DelimSublistPred with just `map (==)`. And is the distinction between the DelimEltPred and DelimSublistPred then still needed at all?

Does it not make sense then, to reuse the DelimSublistPred variant with a singleton list for the DelimEltPred case? (I suppose empty lists must be excluded anyway).

newtype Delimiter a = Delimiter [a -> Bool]

Yes, in fact, Twan sent another message with some code using this exact definition of Delimiter, and I have already made this change to the library. -Brent

On Sat, Jul 21, 2012 at 09:24:16AM +0300, Roman Cheplyaka wrote:

Regarding the API: I'm a bit concerned with presence of synonyms there (e.g. chunk = splitEvery, sepBy = splitOn etc.) IMO it makes it harder to learn the API (which is not small even without the synonyms), and especially to read other people's code if their preferences in naming differ from yours.

Would your concern be addressed by Gábor's suggestion to group synonyms together in the documentation? -Brent

On Sat, Jul 21, 2012 at 11:40:19PM +0300, Roman Cheplyaka wrote:

* Brent Yorgey [2012-07-21 08:09:09-0400]

...

On Sat, Jul 21, 2012 at 09:24:16AM +0300, Roman Cheplyaka wrote:

...

Regarding the API: I'm a bit concerned with presence of synonyms there (e.g. chunk = splitEvery, sepBy = splitOn etc.) IMO it makes it harder to learn the API (which is not small even without the synonyms), and especially to read other people's code if their preferences in naming differ from yours.

Would your concern be addressed by Gábor's suggestion to group synonyms together in the documentation?

This could be a minor improvement (I haven't checked how these grouped functions look like in haddock), but I don't see the need for synonyms in the first place. Could you maybe explain the motivation behind them?

Certainly. The idea is to provide synonyms whenever there are multiple common names in use, as well as a consistent system of names within the package itself. The goal is for new users to be able to get started using the library as quickly as possible -- users will usually come looking for some particular function and they may already have an idea about what it might be called. To be concrete, the split package has three sets of synonyms: * splitOn / sepBy / unintercalate Here 'splitOn' is an internally consistent name, which matches with the naming scheme used in the rest of the package. 'sepBy' is a name from parsec and other parser combinator libraries; 'unintercalate' emphasizes that this function is right inverse to 'intercalate'. * splitOneOf / sepByOneOf * splitEvery / chunk Again, 'splitEvery' matches the internal naming scheme; 'chunk' is a name commonly used for this function within the community. I don't see much harm in this (modulo making the documentation clearer, which I plan to do). And I really don't want to *remove* existing names because that would force a major version bump and potentially break any code depending on split. -Brent

Ben Moseley writes:

On the naming side of things, I'd just like to say I agree with Roman here - I think having synonyms in an API is a bad idea.

Agreed

I think there might be an argument that synonyms improve things for those who know the API really well - but that comes at the cost of worsening the experience for those (i.e. most) who know it moderately or fairly well and thus - when reading code written by others - find themselves struggling to remember whether the variants that they don't use personally are subtly different or not.

Exactly. And it’s very easy for someone who wants a synonym to define it. There’s an issue in this case of avoiding breaking code, but including them, deprecating them and hiding them from the documentation seems to solve that.

P.S. I think "intercalate" is an awful name,

What have you got against it? For the record I was against introducing a name for such a short function (argument is similar to that against synonyms), but it does mean exactly the right thing.

and "unintercalate" is certainly no better ;-)

I’ll grant that that is very ugly. -- Jón Fairbairn Jon.Fairbairn@cl.cam.ac.uk

Hi, not sure if that adds anything to the discussion, but I share Roman's concerns. On Sat, Jul 21, 2012 at 08:43:05PM -0400, Brent Yorgey wrote:

On Sat, Jul 21, 2012 at 11:40:19PM +0300, Roman Cheplyaka wrote:

...

* Brent Yorgey [2012-07-21 08:09:09-0400]

...

On Sat, Jul 21, 2012 at 09:24:16AM +0300, Roman Cheplyaka wrote:

...

Regarding the API: I'm a bit concerned with presence of synonyms there (e.g. chunk = splitEvery, sepBy = splitOn etc.) IMO it makes it harder to learn the API (which is not small even without the synonyms), and especially to read other people's code if their preferences in naming differ from yours.

Would your concern be addressed by Gábor's suggestion to group synonyms together in the documentation?

This could be a minor improvement (I haven't checked how these grouped functions look like in haddock), but I don't see the need for synonyms in the first place. Could you maybe explain the motivation behind them?

Certainly. The idea is to provide synonyms whenever there are multiple common names in use, as well as a consistent system of names within the package itself. The goal is for new users to be able to get started using the library as quickly as possible -- users will usually come looking for some particular function and they may already have an idea about what it might be called.

This was at least not true for me. When I first looked at split, I was utterly confused about all those synonyms. This was one of two reasons why I decided not to use it (having an other dependency was the other).

To be concrete, the split package has three sets of synonyms:

* splitOn / sepBy / unintercalate

Here 'splitOn' is an internally consistent name, which matches with the naming scheme used in the rest of the package. 'sepBy' is a name from parsec and other parser combinator libraries; 'unintercalate' emphasizes that this function is right inverse to 'intercalate'.

* splitOneOf / sepByOneOf

* splitEvery / chunk

Again, 'splitEvery' matches the internal naming scheme; 'chunk' is a name commonly used for this function within the community.

I don't see much harm in this (modulo making the documentation clearer, which I plan to do).

Making the documentation clearer would mitigate Roman's first concern (learning the API). But it would not address the second concern (reading/understanding other peoples code), which to me is even more important.

And I really don't want to *remove* existing names because that would force a major version bump and potentially break any code depending on split.

I think this particular point can be addressed by either deprecating synonyms; or by removing the Haddock comment from synonyms and use: {-# OPTIONS_HADDOCK prune #-} That way the synonyms do not show up in documentation, but are still exported. Cheers, Simon

On Tue, Jul 24, 2012 at 03:13:08PM +0200, Twan van Laarhoven wrote:

On 20/07/12 22:35, Brent Yorgey wrote:

...

Hello everyone,

This is a proposal for the split package [1] to be included in the next major release of the Haskell platform.

Missing strategies ------------------

The specific way that the generic splitting algorithm is implemented does preclude some imaginable splitting strategies. For example, a few years ago I tried adding a strategy that used a predicate on pairs of elements, splitting down the middle of any pairs that satisfy the predicate, but gave up because it simply did not fit.

I just needed a function for splitting a list into two equal parts. This is something you often need for MergeSort-like functions. Is that something that belongs in Data.List.Split?

Perhaps, though I think this sort of thing should probably be handled outside the HP review process. Patches welcome. =) -Brent