-
Notifications
You must be signed in to change notification settings - Fork 21
Add references for pragmas #50
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed the CLA. |
CLAs look good, thanks! |
Seems like GHC 7.10 is not happy: https://travis-ci.org/google/haskell-indexer/builds/255296634?utm_source=github_status&utm_medium=notification [4 of 5] Compiling Language.Haskell.Indexer.Backend.Ghc ( src/Language/Haskell/Indexer/Backend/Ghc.hs, dist/build/Language/Haskell/Indexer/Backend/Ghc.o ) For the time being, could you add ifdefs (see the file for others)? |
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.
Thank you for this PR! Please see some comments.
@@ -33,6 +33,7 @@ import qualified Bag as GHC | |||
import qualified BasicTypes as GHC | |||
import qualified DataCon as GHC | |||
import qualified Name as GHC | |||
import BooleanFormula |
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.
See outer comment about GHC 7.10 failing.
namesFromBooleanFormula :: BooleanFormula a -> [a] | ||
namesFromBooleanFormula bf = | ||
case bf of | ||
Var a -> [a] |
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.
Optional: Travelsing this AST seems pretty compact, but consider using *plate. For example:
let allNames = universeBi aBooleanFormula
This might also help unifying changes across GHC versions, trading a bit of performance (which could be gotten back if we would use Data.Data.Lens
and pull in lens
, but benchmarking needed).
@@ -506,7 +507,29 @@ refsFromRenamed ctx declAlts (hsGroup, _, _, _) = | |||
TypeSig names _ _ -> | |||
#endif | |||
mapMaybe (\(L l n) -> give ctx (nameLocToRef n TypeDecl l)) names | |||
(PatSynSig lnames _) -> |
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.
See universeBi comment below, maybe you can get away with a catch-all clause + biPlate, like
other -> (...map magic with nameLocToRef...) universeBi other
@@ -506,7 +507,29 @@ refsFromRenamed ctx declAlts (hsGroup, _, _, _) = | |||
TypeSig names _ _ -> | |||
#endif | |||
mapMaybe (\(L l n) -> give ctx (nameLocToRef n TypeDecl l)) names | |||
(PatSynSig lnames _) -> | |||
lkupSigNames [lnames] | |||
(FixSig (FixitySig lnames _)) -> |
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.
If these are cases which were not yet crosslinked, could you also update the tests? At least the GHC backend tests at https://github.com/google/haskell-indexer/blob/master/haskell-indexer-backend-ghc/tests/Language/Haskell/Indexer/Backend/Ghc/Test/TypeLinkTestBase.hs, and optionally the GHC+Kythe end-to-end tests - see https://github.com/google/haskell-indexer/tree/master/kythe-verification/testdata/basic, though for GHC+Kythe the type-link tests are not yet ported.
@mpickering do you plan to flesh out this change, or would you prefer if someone took over? |
This patch adds reference for things like
INLINE
pragmas.I tested it by running the indexer on
lens
and observing that theINLINE
pragmas are now linked as expected.