On 29.10.2014 20:50, Alex Henrie wrote:
2014-10-29 6:45 GMT-06:00 Michael Stefaniuc mstefani@redhat.com:
But the code still could look a lot better and some of the previous feedback wasn't addressed. There is an impedance mismatch between the code looking good for Alex and looking good for Wine.
As far as I know, I only rejected one of your suggestions for these 4 patches: Merging the simple_test and complex_test structs. The 3 simple tests are actually used as inputs for 8 tests each, for a total of 24 tests. Reformatting the 3 simple tests as complex tests would force me to manually input the 24 combinations into the complex_tests array. Is that really what Alexandre wants?
-Alex
It is not really true that this was the only thing we criticized. We also gave you a lot more suggestions for improvements, but from the beginning I had the impression that you want to keep changes as minimal as possible. Thats not a good attitude when trying to upstream code, and will probably be especially a problem for the implementation itself. You always have to consider multiple methods how a specific problem can be solved.
For the tests: The main issue (bad test coverage) is fixed at my opinion, but there are still lot of small things which could be improved to make the code more readable. Most of it was already told on IRC, but I will go over patch 1 one more time and tell you again.