-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accept a leading single quote for data constructor promotion #13
Accept a leading single quote for data constructor promotion #13
Conversation
…ntax was not previously understood by hsc2hs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this, @andrewthad.
Question: have you tested these changes with a promoted data constructor that itself ends with a single quote? I ask since there's a special lexical trick you have to employ to promote those:
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE GADTs #-}
data Foo = Foo' | Bar
data Indexed ix where
Indexed :: Indexed ' Foo'
main :: IO ()
main = print $ #size int
Now that I take a look, it appears that all hsc2hs
tests are within GHC itself, not within this repo, which is a bit of shame. Would you mind adding some tests for this feature after this patch lands?
Some comments inline.
…ta constructors that end with single quote.
I cannot figure out how to reply to a review, so I'll do it here. Good point about:
I had never encountered that before. I've made another tweak, it should now handle this case correctly. I've tested it out locally. I'll get the sample code I've added in the note into GHC's test suite whenever GHC bumps to using a version of hsc2hs that has this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is looking clearer to me. Some more questions/comments inline.
HSCParser.hs
Outdated
'\'':'\\':_ -> do any2Chars_; hsString '\''; text | ||
'\'':d:'\'':_ -> do any3Chars_; text | ||
'\'':' ':_ -> do any2Chars_; | ||
manySatisfy_ (\c' -> isAlphaNum c' || c' == '_' || c' == '\'') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might there be other whitespace characters besides
(e.g., Unicode whitespace) that we'd have to consider? Similarly, is isAlphaNum
sufficient to parse everything, or would Unicode characters add an additional wrinkle to this?
Disclaimer: I never use Unicode when writing Haskell, so I have no idea if this is a well formed thought. But I'm sure that if we get this wrong, someone who //does// use Unicode will complain about this at some point :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope that isAlphaNum is sufficient for all of the characters that could show up in an identifier. It’s what this library already uses in other places when it’s parsing identifiers. (So, if it didn’t work, I suspect that someone else would have already complained about it since this library would be broken for non-ascii content). But, I think that it is correct.
The question about alternative space characters is a good one. I’m not familiar with what GHC accepts here. I suspect that GHC probably accepts any number of white space characters of any kind. So, I think I’m currently doing the wrong thing right here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this to the extent that my knowledge permits. I've assumed that in source haskell, anything that isSpace
returns True
for is considered whitespace.
… improve comments on this subject
Great! This patch looks to be in great shape. The only other thing that would need to be changed is the Can anyone else (@hvr?) think of any potential issues with this patch? If not, I'll merge this some time soon(-ish). |
I've added a note in the changelog. Thanks for all the prompt feedback! |
I went ahead and merged this. @andrewthad, would you be willing to open up a MR for GHC that:
If we do that, I think we can close Trac #11004 for good. Thanks! |
Resolves #12