Hi,
Wow, so, I thought there would be some back-and-forth, then a decision, then I would go and walk the last mile and then formally submit the patch for review - and now I see that in <2 days all that has passed...
Of course I'll make validate pass, I just didn't even know about it. Likewise, I needed the carrot of 7.8 inclusion dangling before me to start writing the user docs.
One problem, though, is that I'll be on holiday from tomorrow, so I'll only have time to look into this tonight before next weekend. I'll try my best to fix up validate tonight, and I'll write the docs (which I hope will mostly be an editing job on the wiki) next week. How does that sound?
Thanks,
Gergo
Hi Gergo,
Thanks for rebasing your changes. Unfortunately, they do not compile
cleanly with ./validate, which we really need to have working for all
incoming patches.
In particular, ./validate enables -Werror and a slew of warnings that
you won't normally see during development, which greatly aids in
keeping the code clean. One, for example, is that some of your commits
introduce tabs - we ban tabs and validate errors on them!
Another: the problem is that in
https://github.com/gergoerdi/ghc/commit/afefa7ac948b1d7801d622824fbdd75ade2ada3f,
you added a Monoid instance for UniqSet - but this doesn't work
correctly. The problem is that UniqSet is just an alias for UniqFM
(type UniqSet a = UniqFM a), so the instance is technically seen as an
orphan. Orphan instances cause -Werror failures with ./validate
(unless you disable them for that module, but here we really
shouldn't.)
The fix is to write the Monoid instance for UniqFM directly in
UniqFM.hs instead.
Likewise, here's a real bug that -Werror found in your patch in the
renamer (by building with ./validate):
compiler/rename/RnBinds.lhs:744:1: Warning:
Pattern match(es) are non-exhaustive
In an equation for `renameSig':
Patterns not matched: _ (PatSynSig _ _ _ _ _)
Indeed, renameSig in RnBinds doesn't check the PatSynSig case! The
missing instance looks straightforward to implement, but this could
have been a nasty bug waiting.
If you could please take the time to clean up the ./validate failures,
I'd really appreciate it. I imagine it'll take very little time, and
it will make merging much easier for me. An easy way to do it is just
to check out your pattern-synonyms branches, then say:
$ CPUS=X sh ./validate
where 'X' is the number of cores, similar to 'make -jX'
If it fails, you can make a change, and keep going with:
$ CPUS=X sh ./validate --no-clean
and rinse and repeat until it's done.
Note the --no-clean is required, since `./validate` will immediately
run `make distclean` by default if you do not specify it.
On Tue, Jan 7, 2014 at 5:50 AM, Dr. ERDI Gergo <gergo@erdi.hu> wrote:
> On Mon, 6 Jan 2014, Carter Schonwald wrote:
>
>> as long as we clearly communicate that there may be refinements / breaking
>> changes
>> subsequently, i'm all for it, unless merging it in slows down 7.8 hitting
>> RC . (its
>> taken long enough for RC to happen... don't want to drag it out further)
>
>
> If that helps, I've updated the version at https://github.com/gergoerdi/ghc
> (and the two sister repos https://github.com/gergoerdi/ghc-testsuite and
> https://github.com/gergoerdi/ghc-haddock) to be based on top of master as of
> today.
>
> Bye,
> Gergo
>
> --
>
> .--= ULLA! =-----------------.
> \ http://gergo.erdi.hu \
> `---= gergo@erdi.hu =-------'
> Elvis is dead and I don't feel so good either.
> _______________________________________________
> ghc-devs mailing list
> ghc-devs@haskell.org
> http://www.haskell.org/mailman/listinfo/ghc-devs
>
--
Regards,
Austin - PGP: 4096R/0x91384671