AMP (#8004) almost finished, review would be nice
simonpj at microsoft.com
Tue Sep 3 16:19:35 CEST 2013
Thanks for doing this.
Some comments on the code.
* Use warnTc or addWarnTc, rather than fiddling with mkPlainWarnMsg etc.
* Ditto in checkShouldInst, instead of returning a (Maybe WarnMsg),
just use warnTc inside checkShouldInst.
* Don't use tryTc in tcLookupClassMaybe -- it's a sledgehammer
to crack a nut. The only classes you are looking up are monad,
applicative, alternative, and they will always be found -- or at
least if not we have a problem, and tcLookupClass will rightly
report an error. So I see no need for this tryTc and matching
* Why not just getInstEnvs and lookupInstEnv in checkShouldInst,
much as in TcInteract.matchClassInst? Instead you are passing
parameters around that are readily available in the Tc monad.
* I don't see any documentation.
I don't know about this validation stuff; maybe others can help. Why not *not* add -fno-warn-amp to GcStage1HcOpts?
| 1. Validation does not work due to the warnings issued. Since "sh
| validate" uses -Werror, an AMP warning will stop the compilation before
| tests can even be run. Unfortunately, the build system seems to use the
| variables 'GhcStage1HcOpts' in the build process of phase 1, which is of
| course done with the current (7.6.3 or whatever is installed) compiler.
| When adding "-fno-warn-amp" to that variable phase 1 won't build because
| the parameter is unknown to the old compiler.
Is there some sort of
| -"ignore next parameter if unknown" hack, or is there a smart solution?
| 2. Temporarily removing the -Werror constraint from validate-settings.mk
| (or by using custom-settings.mk) makes the validation build go through.
| However, the testsuite defines a couple of violating modules, therefore
| there is unexpected STDERR output, hence a handful of tests fail. Should
| a fix for this be included in the AMP patch, or be done as a separate
| 3. Similarly, GHC defines around 50 offending modules, creating warnings
| in the standard build process. Again, should this be included, or can we
| push that to after the feature freeze and regard it as bugfixing?
| ghc-devs mailing list
| ghc-devs at haskell.org
More information about the ghc-devs