Rename literal constructors
Needs ReviewPublic

Authored by hsyl20 on Jun 21 2018, 2:51 AM.

Details

Summary

In a previous patch we replaced some built-in literal constructors
(MachInt, MachWord, etc.) with a single LitNumber constructor.

In this patch we replace the Mach prefix of the remaining constructors
with Lit for consistency (e.g., LitChar, LitLabel, etc.).

Sadly the name LitString was already taken for a kind of FastString
and it would become misleading to have both LitStr (literal
constructor renamed after MachStr) and LitString (FastString
variant). Hence this patch renames the FastString variant PtrString
(which is more accurate) and the literal string constructor now uses the
least surprising LitString name.

Both Literal and LitString/PtrString have recently seen breaking
changes so doing this kind of renaming now shouldn't harm much.

hsyl20 created this revision.Jun 21 2018, 2:51 AM
tdammers requested changes to this revision.Jun 21 2018, 10:45 AM
tdammers added a subscriber: tdammers.

Apart from the stylistic remarks; I wonder whether this change would pull its weight. The Mach- prefix doesn't seem entirely outrageous to me (after all, these are things that are supposed to be "close to the metal" or "in machine format"), and changing them has a bit of an impact wrt future patches, merging, etc. But I really can't judge which way the balance would tip.

compiler/basicTypes/Literal.hs
109–110

Messed up the alignment here

110

...and here too

489–493

I don't understand this comment; I bet there is some meaning to it, but can we maybe figure out what it is and rephrase it in a less cryptic way?

compiler/codeGen/StgCmmUtils.hs
103

Consider fixing alignment here, since we're touching the code anyway.

compiler/coreSyn/MkCore.hs
76

Why? Is there another function mkPtrString anywhere? If so, then wouldn't that mean that this entire patch just trades one naming clash for another?

compiler/deSugar/MatchLit.hs
380

Shouldn't that be LitBytes instead of MachBytes here?

This revision now requires changes to proceed.Jun 21 2018, 10:45 AM

That's a lot of bits flipped for no functional changes :) I also wonder whether this is worth it. What is it about the Mach prefix that bothers you?

Indeed I am also a bit skeptical of the value of this patch. However, if we want to put it in then we need to do so soon otherwise it will be a nightmare to cherry-pick around.

hsyl20 updated this revision to Diff 17050.Jun 22 2018, 8:11 AM
hsyl20 marked 6 inline comments as done.

Indeed it is only a cosmetic change but the Mach prefix always seemed weird to me. The only machine-specific literals are MachInt, MachWord and MachNullAddr but two of them have already been renamed. All the other ones are specified independently of the actual machine (Float, Double, Word32, Word64, Label, etc.) and/or are not so close to the machine (Unicode char, UTF8 string, Integer, Natural).

If the datatype was named "MachineLiteral" or something like that it would make sense to have a Mach prefix, but I think it would be clearer to drop the misleading (at least for me) "machine" reference instead.

Finally, I would probably not have proposed this patch if MachInt/... hadn't been removed in a recent patch. Initially I even added pattern synonyms to avoid breaking them but SPJ argued to remove them altogether. So, since we already break Literal in the next release, I figured we could do use this window to refactor them a bit.