Pretty-print # on unboxed literals in core
ClosedPublic

Authored by thomie on Feb 23 2015, 12:50 PM.

Details

Summary

Ticket Trac #10104 dealt with showing the '#'s on types with unboxed fields. This
commit pretty prints the '#'s on unboxed literals in core output.

Test Plan

simplCore/should_compile/T8274

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
thomie updated this revision to Diff 2296.Feb 23 2015, 12:50 PM
thomie retitled this revision from to Pretty-print # on unboxed literals in core.
thomie updated this object.
thomie edited the test plan for this revision. (Show Details)
thomie added reviewers: austin, jstolarek.
thomie updated the Trac tickets for this revision.
thomie added a subscriber: thomie.
thomie added inline comments.Feb 23 2015, 12:57 PM
compiler/hsSyn/HsLit.hs
156

This was a bug. pprHsCar already appends a '#'.

testsuite/tests/simplCore/should_compile/all.T
202

This change turned up a latent bug in T8832, but it is not further related. I reopened Trac #8832.

thomie updated this revision to Diff 2297.Feb 23 2015, 1:00 PM

Fix typo

thomie updated this revision to Diff 2298.Feb 23 2015, 1:01 PM

Fix typo

thomie added inline comments.Feb 23 2015, 1:05 PM
compiler/hsSyn/HsLit.hs
156

Sorry, I meant: pprHsBytes s <> char '#' was a bug, pprHsBytes already appends a '#'.

thomie updated this revision to Diff 2299.Feb 23 2015, 2:03 PM

Update test output

austin accepted this revision.Feb 24 2015, 5:08 AM
austin edited edge metadata.

lgtm

This revision is now accepted and ready to land.Feb 24 2015, 5:08 AM
simonpj accepted this revision.Feb 26 2015, 11:17 AM
simonpj added a reviewer: simonpj.
simonpj added a subscriber: simonpj.

Generally I'm ok with this. Happy to accept it. One comment above to attend to.

Thanks

Simon

compiler/basicTypes/Literal.hs
450–456

This used to call pprIntVal which used parens for negative literals. You removed that. Why?

Mind you, it was terribly inconsistent before. Eg negative doubles did not have the same treatment.

Simon

thomie added inline comments.Feb 26 2015, 2:37 PM
compiler/basicTypes/Literal.hs
450–456

Negative unboxed integers will now be printed as: -1#. This doesn't require parenthesis in any context with GHC. For example, the following is accepted by the lexer: 1# +# -1# (unlike 1 + -1). So I decided not to print the parenthesis either.

Note that LitInteger will still get parenthesis when needed. That is what add_par does. Also note that a LitInteger will now be printed as is, without any prefix ("__integer") or postfix modifier, since that syntax gets freed up with unboxed integers using the '#' suffix.

OK. Could you add a Note in Literal.hs with a little table showing, for each type, how it is printed. That'll help readers I think, and would take but a moment. Thanks

Simon

thomie updated this revision to Diff 2325.Mar 2 2015, 3:31 AM
thomie edited edge metadata.

Add Note [Printing of literals in core]

Helpful Note; I've added a question to clarify

compiler/basicTypes/Literal.hs
463

Very helpful. Can you just add a sentence explaining why -1# is "atomic" but -1 is not?

I think perhaps you are reflecting something about GHC's parser for Haskell? If so, better to say so. It's not an obvious link, since this is Core, and GHC's Haskell parser will never see it.

Or maybe it's some other reason?

Simon

thomie updated this revision to Diff 2333.Mar 2 2015, 2:04 PM

Three changes from previous revision:

  • bring back pprIntegerVal to distinguish between positive and negative integers
  • don't print parenthesis around MachInt64 and MachWord64 either
  • try to explain better when parenthesis are printed, and why.

Improving all the time but I'm still puzzled by LitInteger

compiler/basicTypes/Literal.hs
492

Comment is helpful, but why is LitInteger different? What does "context requires an atom" mean?

thomie added inline comments.Mar 2 2015, 3:46 PM
compiler/basicTypes/Literal.hs
492

At the top of the Note it says: "if they occur in a context requiring an atomic thing (for example function application".

LitInteger is different because I print LitInteger in Core using plain numbers (no prefix or suffix), and plain negative literals in Haskell require parenthesis in contexts like function application. I'm trying to mimic Haskell in the output for Core, to make it easier for a Haskell user to read Core. (That's also the reason I started working on this ticket, I find Core more difficult to read then necessary).

We could also choose to not print parenthesis around LitInteger -1 in core, but then a Haskell user might get confused.

off-topic: Mind you, I hate that 1 - -1 is not accepted in Haskell. It is the primary reason I still use Python, which does accept it, instead of ghci as my go-to calculator.

thomie added a comment.Mar 2 2015, 3:59 PM

We could simplify by always printing parenthesis around negative LitInteger and MachLabel, then the function add_par isn't needed, and there isn't any context to explain.

In D678#19347, @thomie wrote:

We could simplify by always printing parenthesis around negative LitInteger and MachLabel, then the function add_par isn't needed, and there isn't any context to explain.

Either is fine with me. But if you retain the add_par stuff maybe you can transfer the explanation of your previous comment to the Note.

thomie updated this revision to Diff 2340.Mar 3 2015, 3:01 AM

Expand Note with previous comment

This revision was automatically updated to reflect the committed changes.