Previously, we were using foldl1 instead, which led to the derived
code to be wrongly associated.
Details
Details
./validate
Diff Detail
Diff Detail
- Repository
- rGHC Glasgow Haskell Compiler
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| compiler/typecheck/TcGenDeriv.hs | ||
|---|---|---|
| 219 | Maybe phrase the comment in a way that it talks positively about the current code? (“Using foldr causes correctly associated cdoe”) Future readers of the comment don’t know much about the history. | |
| compiler/typecheck/TcGenDeriv.hs | ||
|---|---|---|
| 219 | You're right, this does read better. | |
Comment Actions
Arg, Phab ate half my comments :-(
I am not sure about the test case. Without -dsuppress-uniques, the output will change a lot. And even with I am not sure of the merits of just dumping the whole output: It will still change as people make unrelated changes, and then the developer will have to approve the new output, likely without knowing what to look for.
A testcase that actually declares the properties we are looking for would be far more difficiult to write than this issue warrants.
So I am inclined to just not have a test case for this issue.
Comment Actions
Oh right, I should've used -dsuppress-uniques. Would a comment in the test file help ? Otherwise, I'll update this patch to not include it.
A testcase that actually declares the properties we are looking for would be far more difficiult to write than this issue warrants.
Out of curiosity, how can I write such a test ? Anything particular that I should grep to find other tests that do something similar ?
Comment Actions
Yeah, comments in the test file are always a good idea (although it still requires the developer to look there – initially they only see the differences in the output)
A testcase that actually declares the properties we are looking for would be far more difficiult to write than this issue warrants.
Out of curiosity, how can I write such a test ? Anything particular that I should grep to find other tests that do something similar ?
You can grep for grep, some tests declare what to look for the in the dump. But in this example, I think it is hard to write a reliable grep command. One could use the GHC API to inspect the desugared Core, but for this issue at hand that’d be great overkill.
Comment Actions
You can grep for grep, some tests declare what to look for the in the dump. But in this example, I think it is hard to write a reliable grep command.
Interesting. I had not come across those before. Good to know for future patches that might need it.
One could use the GHC API to inspect the desugared Core, but for this issue at hand that’d be great overkill.
Agreed.
I hope I did not discourage you from writing test cases in the future :-)
Not at all! Thank you for reviewing the patch :)