Compiled ghc fine. Opened ghci and fed it invalid code. It gave the improved error messages in response.
There's some obvious extra junk in the diff like the deriving Show bits. I didn't think I was going to be uploading anything when messing around with arc, and now I don't know how to edit the diff :)
Your diff includes a bunch of unrelated whitespace-only changes. Could you remove these? Also there are some other changes that appear to be unnecessary.
The new error messages look nice, though I'm not enough of an expert on GHC's parser to know whether the implementation is correct.
|490 ↗||(On Diff #513)|
You don't seem to use this (probably it was added for debugging?)
... or these
I don't think you use this new argument either?
This change doesn't change anything
|2393–2396 ↗||(On Diff #513)|
Delete this if it isn't used
Oh, I didn't see your comment. You can amend your commit (or just add more commits on top--arc will squash them for review) and rerun arc diff, and it should locate the corresponding Differential and update it automatically.
- There's still some spurious whitespace changes. But I think all these are from lint wanting me to remove the extra whitespaces. Is that okay, or should I remove them? Also, why is lint configured to have all these whitespace and line too wide warnings if so much of the codebase doesn't follow the rules?
- The commits don't pass the build test because the tests assume the old error messages and the new errors don't match. Is there any easy way to update these tests without manually copying and pasting a bunch of stuff?
- As for the correctness, the part I'm least sure about is the let statements. It seems to work for lets in the non-monadic case and doesn't interfere with the monadic lets. There might be some edge case I'm not considering though, and the applicative do might be different somehow.
- You should just be able to fix up whitespace in the area you are touching. You shouldn't need to fix up the whole file. That makes it hard to find the actual functional changes. We want to eventually remove all tabs and trailing whitespace, so the lint rules are there to avoid their introduction in new code. But we don't want to fix up the whitespace all at once because that will cause a lot of extra work for people who have large changesets on a branch. (Though we have considered doing so anyways.)
- Yes, see https://ghc.haskell.org/trac/ghc/wiki/Building/RunningTests/Updating.
This syntactic change does nothing, should be removed
s is unused here, remove it (normally validate would fail with an unused variable warning, but we disable warnings in this file as it gets incorporated into generated code)
Is there an easy way to tell arc to ignore the changes on certain lines in a file? I'm trying to figure out how to undo all the whitespace changes without opening up an editor and manually retyping everything back in.
Basically fine, thank you for taking time to help
I'll take the parser stuff on trust! I've added a note about TcErrors.
Better English "maybe you have not applied a function to enough arguments)"
I believe (but it's hard to tell) that this is the only significant change in this file. Can you add a Note to explain the intent, give an example, and link to Trac Trac #9613?
If I were you I would fix your commit in git as follows:
- git reset HEAD^ to "uncommit" the commit, but leave the changes in the working copy
- git add -p and add only the changes you want (the non-whitespace changes)
- git diff --staged to make sure you didn't stage any whitespace changes
- git diff to make sure you didn't fail to commit any real changes (I suppose git diff -b should produce no output, then)
- git commit -C 73944a768449 to create a new commit with the same commit message as your original commit 73944a768449
- git checkout -p to throw away the whitespace changes in your working copy
- arc diff to update this Differential