Decide on coding style

[michalt]

Hi all,

When reading through the library I often find that inconsistent indentation and coding style is making things more difficult to read and understand. A few examples:

• IIRC I've seen code indented with anything between 1 to 4 spaces.
• Do blocks often use braces/semicolon syntax instead of the more accepted (and IMHO far more readable) syntax based on indentation. Although both are used in various places.
• Very long lines.
• Trailing whitespace.

So I was wondering if we could simply agree to some already accepted coding style, e.g.,
https://github.com/tibbe/haskell-style-guide/blob/master/haskell-style.md

What do you think? (I'd be happy to send PRs to convert at least some of the code to the new style, e.g., Dataflow.hs which is non-trivial)

[jstolarek]

Yes, I'd be all for consistent code style. I vote for 2-space indentation. I have no opinion yet about do-notation style (indentation vs. braces). In fact braces style dominates in GHC source code. It seems to be Simon PJ's preferred style and that is most likely explanation why it appears in the library. I initially disliked that style but the truth is that for very long do-block this is really easier to read.

And of course I'm all for nuking trailing whitespaces and shortening the lines (where reasonably possible).

[michalt]

I guess we could just follow the style guide from tibbe (the one I linked above) and just use 2 spaces instead of 4. Would that work for you?

(Side note on the do-notation: GHC is not really consistent itself in this regard either and it really depends on which part you look at, e.g., cmm/ and llvmGen/ seem to mostly use the syntax without braces. I guess that's consistent with SPJ preferring the braces and other people preferring no braces ;-)

[lexpank]

Yeah, I agree that style should be just the same (at least for hoopl) because sometimes reading it is pain, and functional programming considered to be beautiful. I also vote for 2 spaces :)

[jstolarek]

I guess we could just follow the style guide from tibbe (the one I linked above) and just use 2 spaces instead of 4. Would that work for you?

If 4 spaces don't cause too much trouble with too long lines then I'm fine with that. Alternatively, let's use style that requires less changes.

[michalt]

@jstolarek I'd really prefer to avoid creating yet another style (that's one of the not-so-nice things about Haskell packages - they often use very different coding styles). Since tibbe's one is both nicely written and used in a few important packages, it seems like a good candidate to just follow (also if you google for "haskell coding style" it's one of the top results). There's a lot of value in consistency even if some details are not exactly what I'd choose myself (just image how cool it'd be to have something like go-fmt for Haskell :-)

[jstolarek]

I am not proposing to create a new formatting style. I think tibbe's style is what most people use anyway. And that style guide is silent on the do-notation.

Anyway, no strong opinions from me. Any consistent style is better than an inconsistent one.

[michalt]

Note that even though the tibbe's style guide is not explicit about the do-notation, all the examples actually use the indentation based one, not the one with braces.

So I guess everyone is ok with tibbe's style in general? What about indentation? Should we first try 4 spaces and switch to 2 in case of trouble or do people feel strongly about using 2 spaces?

[jstolarek]

Let's try 4 spaces

[mlite]

I want to minimize the changes to this library. If a change does not improve usability or add new functionality, please don't make the change.

[jstolarek]

I want to minimize the changes to this library.

Why? If we can make the code better why not do so?

If a change does not improve usability or add new functionality, please don't make the change.

Doesn't cleaner code improve things?

But if you really don't want to introduce a consistent coding style then let us at least nuke trailing whitespaces. This is mostly annoying, especially that hoopl is used by GHC and GHC itself went through major code cleanups (including trailing whitespace removal) some time ago.

[mlite]

I have a very conservative approach to maintaining the library. It
works very well for the use cases it was designed for. Unless there are
more convincing use cases, I would like to keep it to the current
form. To my best knowledge, it hasn't seen a wider adoption outside of
GHC yet. I'm hesitant to make changes that are not driven by use
cases or bug reports.

"cleaner code" is subjective.

On 1/18/2016 11:04 PM, Jan Stolarek wrote:

I want to minimize the changes to this library.
Why? If we can make the code better why not do so?

If a change does not improve usability or add new functionality, please don't make the change.
Doesn't cleaner code improve things?

But if you really don't want to introduce a consistent coding style then let us at least nuke trailing whitespaces. This is mostly annoying, especially that hoopl is used by GHC and GHC itself went through major code cleanups (including trailing whitespace removal) some time ago.

---

Reply to this email directly or view it on GitHub:
#24 (comment)

[jstolarek]

Then let us at least clean the trailing whitespaces, like we did with the rest of GHC, shall we?

[michalt]

@mlite TBH I have to disagree here - I don't think the library works well for the use cases it was designed. After all it's not even fully used in GHC due to performance problems (it seems that GHC doesn't use its transformation capabilities, which is the biggest selling point of the library!) [1, 2]. So I wouldn't rule out large scale changes if we want to fix this. Yes, we probably shouldn't start with rewriting everything, but starting with a very conservative approach from the beginning might be too limiting. If you really do prefer to avoid any major changes to Hoopl, we can also try to just (temporarily?) fork it and see what are the benefits and if it makes sense to merge later.

[1] http://mail.haskell.org/pipermail/ghc-devs/2013-October/003120.html
[2] https://plus.google.com/107890464054636586545/posts/dBbewpRfw6R

[mlite]

I don't get why these trialling whitespaces are significant. If yes,
in what sense?

On 1/18/2016 11:21 PM, Jan Stolarek wrote:

Then let us at least clean the trailing whitespaces, like we did with the rest of GHC, shall we?

---

Reply to this email directly or view it on GitHub:
#24 (comment)

[mlite]

Perhaps, I didn't clarify my position well. I support changes backing
by use cases. You actually listed a convincing use case to support some
changes, don't you think so?

BTW, I don't recall the paper mentioned high performance is one design
consideration. It was designed for doing complex interleaving analyses
and transformations. High performance and generality don't generally
reconcile well.

On 1/18/2016 11:35 PM, Michal Terepeta wrote:

@mlite TBH I have to disagree here - I don't think the library works well for the use cases it was designed. After all it's not even fully used in GHC due to performance problems (it seems that GHC doesn't use its transformation capabilities, which is the biggest selling point of the library!) [1, 2]. So I wouldn't rule out large scale changes if we want to fix this. Yes, we probably shouldn't start with rewriting everything, but starting with a very conservative approach from the beginning might be too limiting. If you really do prefer to avoid any major changes to Hoopl, we can also try to just (temporarily?) fork it and see what are the benefits and if it makes sense to merge later.

[1] http://mail.haskell.org/pipermail/ghc-devs/2013-October/003120.html
[2] https://plus.google.com/107890464054636586545/posts/dBbewpRfw6R

---

Reply to this email directly or view it on GitHub:
#24 (comment)

[jstolarek]

I don't get why these trialling whitespaces are significant. If yes, in what sense?

I think all the problems boil down to stating that trailing whitespaces are semantically irrelevant to humans but that are relevant to computers. Adding or deleting trailing whitespaces shows up as noise in git commits. Many people configure their editors to delete trailing whitespaces on save to prevent introducing accidental changes. Such people would have hard time working with hoopl - they would have to disable trailing whitespace removal or otherwise their commits would contain noise. I personally have Emacs configured in such a way that trailing whitespaces are highlighted in red, which alerts me of the problem but does not fix it automatically.

Of course a good next step would be to add a hook that prevents pushing code that adds trailing whitespaces. I believe GHC repository has such a hook.

[michalt]

@mlite Ok, how about this:

1. We adopt tibbe's style for all new code.

2. When changing a file (in some non trivial way), we also allow to have a separate commit that fixes the formatting of that file according the style guide.

This way we avoid any big "reformatting" for its own sake, but do converge on the coding style over time (as we modify the functionality or performance of the code).

Does that sound ok for you guys?

[jstolarek]

Yes from me, except that:

all new code

this doesn't really happen :-) No one seems to be adding much new code to hoopl. What IMO hoopl needs is a decent code cleanup to get rid of some mess in the code and fix some bad design decisions. I have one of my students working on that at the moment - hopefully something good will come of it :-)

[mlite]

Reformatting code is never perfect, it could create a big burden to code
auditing.

I like the incremental plan, but please keep in mind avoiding formatting
code such that we lose the track of what the real changes are made.

On 1/19/2016 9:59 AM, Michal Terepeta wrote:

@mlite Ok, how about this:

1. We adopt tibbe's style for all new code.

2. When changing a file (in some non trivial way), we also allow to have a separate commit that fixes the formatting of that file according the style guide.

This way we avoid any big "reformatting" for its own sake, but do converge on the coding style over time (as we modify the functionality or performance of the code).

Does that sound ok for you guys?

---

Reply to this email directly or view it on GitHub:
#24 (comment)

[mlite]

hoopl is released as an independent hackage, it has users other than GHC.

Because hoopl is released with GHC, the hoopl upgrade could have
significant impacts to external users. If incompatible changes go to
hoopl, all external users cannot upgrade GHC without making changes to
their code accordingly.

If you plan to push your changes upstream, we want to be notified what
changes will be made earlier.

On 1/19/2016 10:19 AM, Jan Stolarek wrote:

Yes from me, except that:

all new code
this doesn't really happen :-) No one seems to be adding much new code to hoopl. What IMO hoopl needs is a decent code cleanup to get rid of some mess in the code and fix some bad design decisions. I have one of my students working on that at the moment - hopefully something good will come of it :-)

---

Reply to this email directly or view it on GitHub:
#24 (comment)

[jstolarek]

Because hoopl is released with GHC, the hoopl upgrade could have significant impacts to external users. If incompatible changes go to hoopl, all external users cannot upgrade GHC without making changes to their code accordingly.

Yes, we're talking about backwards-incompatible changes to the API that will require hoopl clients to adjust their code upon upgrade. But these changes are well-justified. Personally, I would not consider impact of these changes to be "significant". Some code surely will need to be updated but it wont be much.

If you plan to push your changes upstream, we want to be notified what changes will be made earlier.

Sure, I understand that. Actually, I can tell you what the likely changes will be. In 2013 I spent summer at Microsoft Research and hoopl was one of the things I was working on. Back then me and Simon Peyton Jones realized that hoopl has some design flaws. These flaws costed me tens of hours of debugging - I wouldn't have wasted this time had hoopl was better designed. These deficiencies were documented on GHC wiki. The changes that are likely to happen sometime soon are:

• removal of Label argument from JoinFun
• changing the definition of FwdPass (no bottom element required since it is never used) and BwdPass (JoinFun and bottom instead of DataflowLattice, to be consistent with FwdPass).

[hvr]

If incompatible changes go to hoopl, all external users cannot upgrade GHC without making changes to their code accordingly.

Minor nitpick as this is not fully accurate: You can easily use a different hoopl version from the one that ships with GHC, unless you also depend on the ghc library. Future cabal versions will help make this a normal state of affairs (but it works just fine already now).

[mlite]

Yes, my code depends on ghc library.
It would be great if the problem can be resolved in the future.

On 1/20/2016 2:57 AM, Herbert Valerio Riedel wrote:

If incompatible changes go to hoopl, all external users cannot upgrade GHC without making changes to their code accordingly.
Minor nitpick as this is not fully accurate: You can easily use a different hoopl version from the one that ships with GHC, unless you also depend on the ghc library. Future cabal will help make this a normal state of affairs.

---

Reply to this email directly or view it on GitHub:
#24 (comment)