Conversation
avh4
changed the title
This also removes -fno-unused-do-binds from modules that had it inline, and enables -Wall for the general build.
robinheghan
left a comment
robinheghan
left a comment
There was a problem hiding this comment.
Great work!
make sure we copy the content from the ByteString buffer before it's deallocated
By copy you mean the Utf8.fromPtr calls, right?
| addLocation (Var.upper E.Effect) | ||
|
|
||
| spaces_em :: Parser E.Module () | ||
| spaces_em :: Parser E.Module [Src.Comment] |
There was a problem hiding this comment.
Why change () to [Src.Comment] here? Is it for easier composition later?
There was a problem hiding this comment.
I didn't really look where spaces_em is used yet, but I just pushed that up since this is just a wrapper for Space.compAndCheckIndent. My plan to making sure we don't accidentally discard any comments is to keep pushing the [Src.Comment] up at every level until we get to the parse that actually creates the AST node, and rely on the unused-bind warning to tell us when there are still comments that are discarded.
| {-# LANGUAGE BangPatterns #-} | ||
| {-# LANGUAGE OverloadedStrings #-} | ||
| {-# LANGUAGE UnboxedTuples #-} | ||
| {-# OPTIONS_GHC -Wall -fno-warn-unused-do-bind #-} |
There was a problem hiding this comment.
It's great that you clean up stuff like this along the way 💯
| chompAndCheckIndent toSpaceError toIndentError = | ||
| P.Parser $ \(P.State src pos end indent row col) cok _ cerr _ -> | ||
| let (# status, newPos, newRow, newCol #) = eatSpaces pos end row col | ||
| let (# status, newPos, newRow, newCol #) = eatSpaces pos end row col [] |
There was a problem hiding this comment.
Not related to your change: what's up with the (# syntax here? I assume it's a special type of tuple. Stack-allocated, maybe?
There was a problem hiding this comment.
I believe it's an unboxed tuple, so I guess that means all four return values will be allocated on the stack, instead of allocating a Tuple object? I've never really used it before seeing it here in the parser.
|
|
||
| data MultiStatus | ||
| = MultiGood | ||
| = MultiGood !(Utf8.Utf8 Src.GREN_COMMENT) |
There was a problem hiding this comment.
The bang operator is a way to opt-out of lazyness, right? Sorry about the noise, just checking my understanding as I'm reading 😅
There was a problem hiding this comment.
Yes. (And also this is something I haven't used much myself apart from seeing it here.) I'm not quite sure if it means that the argument to MultiGood will be evaluated before creating the MultiStatus value, or if it means that the argument will be forced at the same time that the MultiStatus value is evaluated.
Yep. |
2 participants
{- ... -}comments-- ...commentsparse "comment trick" commentsdecided not to do this for now, for future reference, see Multiline comment "trick" not fully supported avh4/elm-format#154Note there are two competing concerns w/r to laziness: 1) make sure we copy the content from the ByteString buffer before it's deallocated, and 2) try not to do extra work if the result is going to be discarded. I think I did an 80% of ideal here, erring on the side of (1). Notably, in the case of DocComments, I'm pretty sure there's double copying that could be cleaned up later; and also maybe the ByteString buffer is deallocated later than I think, in which case we could probably skip the copying by using
P.Snippetinstead ofUtf8.Utf8like DocComment does, if we're confident that the originalByteStringwill still be around until we finish formatting.The next step after this is #58