-
Notifications
You must be signed in to change notification settings - Fork 206
Conversation
@mpickering Anything missing for this PR? Or do you think we should rather wait for your PR to be merged? |
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.
You should quickly check the ghc-mod
source to see if there are any other hover points it supports so this doesn't regress behaviour?
Also, are there any tests for hover? It would be good to add some if there aren't but it shouldn't block merging this MR.
Will do, thanks!
I think there are some tests, but I will investigate |
be44bc9
to
ffcbe45
Compare
ffcbe45
to
1805ebb
Compare
ee3f3fa
to
823d35e
Compare
4eec139
to
a81a265
Compare
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 this @fendor the patch looks really good now. The tests are a big improvement as well!
pattern HsOverLitType t <- | ||
#if MIN_VERSION_ghc(8, 6, 0) | ||
GHC.HsOverLit _ (GHC.overLitType -> t) | ||
#elif MIN_VERSION_ghc(8, 4, 0) |
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.
Can just have the else
clause here I think.
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.
Yeah, I wanted to ask that anyway, do we prefer easy to read, e.g. always three clauses although they often completely overlap, or conciseness when it comes to CPP?
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 prefer not having the duplication and just having two clauses when possible.
When the tests run through, this should be ready to merge, imo. |
Given what a big change this is, I suggest we merge it after the April release, which is due in a few days. |
CI looks to be failing still on 8.2.2 and 8.2.1. @alanz Are you sure you don't want to merge this? It improves one of the big complaints about HIE which is high memory usage. |
@mpickering Tests fail for the type information for deriving clauses. Perhaps the code generation API has changed over one GHC version to the next? However, failing tests are unrelated to the feature itself. |
720d292
to
998350f
Compare
998350f
to
a5be92e
Compare
CI fails are imo unrelated to this PR. |
This can be merged now I think? |
I think so, I presume the CI is transient. |
In other words, I guess I should actually take a look at it now. |
Under the orchestration of @mpickering, reimplements the
genTypeMap
function in a new module.A Typechecked module is only traveresed once.
This reimplementation should have a lower memory footprint but is likely to increase the first startup time of modules.
Some parts may no longer have a type on hover.
Implementation has already been merged into #1126. There it is said to be working!
Closes #1156