Convert more uses of String to Text.
#1281
Conversation
| @@ -1439,7 +1440,7 @@ setupArg sc sym ecRef tp = do | |||
| freshGlobal cty sc_tp = | |||
| do ecs <- readIORef ecRef | |||
| let len = Seq.length ecs | |||
| ec <- scFreshEC sc ("arg_"++show len) sc_tp | |||
| ec <- scFreshEC sc ("arg_" <> Text.pack (show len)) sc_tp | |||
There was a problem hiding this comment.
There are two calls to Text.pack here, one of which is implicit in the use of OverloadedStrings. This can be shaved down to just one:
| ec <- scFreshEC sc ("arg_" <> Text.pack (show len)) sc_tp | |
| ec <- scFreshEC sc (Text.pack ("arg_" ++ show len)) sc_tp |
There was a problem hiding this comment.
I had assumed that any calls to Text.pack arising from OverloadedStrings would be constant-folded away by the compiler; is this not the case? I was actually thinking that the obvious next improvement for this line would be to find a replacement for pack . show.
There was a problem hiding this comment.
I thought I recalled pack being a performance bottleneck, but now that I look closer, that's for ByteString's pack, not Text's pack.
Both versions will constant-fold the "arg_" part, so I suppose it's just a question of whether it's faster to append two strings and pack them at the end, or to pack two strings and them append them in the end. But for strings this small, it seems very unlikely to matter.
tl;dr Go with whichever style looks better to you.
There was a problem hiding this comment.
I like the idea of minimizing the number of subexpressions of type String, so if the performance is about the same, I'll just keep it as is.
| @@ -202,7 +202,7 @@ buildArg arg idx | |||
| $ arg ^. argSkelType . typeSkelLLVMType | |||
| pure (Just val, Nothing, arg ^. argSkelName) | |||
| where | |||
| ident = maybe ("arg" <> show idx) Text.unpack $ arg ^. argSkelName | |||
| ident = maybe ("arg" <> Text.pack (show idx)) id $ arg ^. argSkelName | |||
There was a problem hiding this comment.
| ident = maybe ("arg" <> Text.pack (show idx)) id $ arg ^. argSkelName | |
| ident = maybe (Text.pack ("arg" ++ show idx)) id $ arg ^. argSkelName |
| @@ -237,7 +237,7 @@ rebuildArg (arg, prearg) idx | |||
| -> LLVM.Array (fromIntegral $ s ^. sizeGuessElems) pt | |||
| | otherwise -> pt | |||
| _ -> pt | |||
| ident = maybe ("arg" <> show idx) Text.unpack nm | |||
| ident = maybe ("arg" <> Text.pack (show idx)) id nm | |||
There was a problem hiding this comment.
| ident = maybe ("arg" <> Text.pack (show idx)) id nm | |
| ident = maybe (Text.pack ("arg" ++ show idx)) id nm |
brianhuffman
force-pushed
the
string-text
branch
from
0996054 to
3303426
Compare
brianhuffman
added
the
PR: ready to merge
This helps with #1269, removing several occurrences of
Text.unpack.