Ivan Gyurdiev ivg231@gmail.com writes:
Changelog:
- Abandon central memory allocation, and the tracking that involves.
Write a setup() and teardown() handler for each test, and use heap allocation.
- Make the main test function configure test arguments.
Replace get/set arguments with a single test_arg.
- Remove some const qualifiers on test data.
Const qualifiers should go on function parameters instead.
When you find yourself writing a Changelog like this, it's a clear sign that it should be 3 separate patches.
Alexandre Julliard wrote:
Ivan Gyurdiev ivg231@gmail.com writes:
Changelog:
- Abandon central memory allocation, and the tracking that involves.
Write a setup() and teardown() handler for each test, and use heap allocation.
- Make the main test function configure test arguments.
Replace get/set arguments with a single test_arg.
- Remove some const qualifiers on test data.
Const qualifiers should go on function parameters instead.
When you find yourself writing a Changelog like this, it's a clear sign that it should be 3 separate patches.
Well, sort of...
I want to add a setup() and teardown() handler.
To do that, I need to give them an argument - it makes sense to make that argument shared with the other functions for simplicity (kind of like a context to the handlers). Now I find that a bug in const handling is causing compiler warnings all over the place (in process of writing a setup/teardown handler), so I'm forced to fix that too.
Essentially the first change drives the other two. Sure, I suppose I could revert the changelog and send #2 and #3 first, then make the change I'm interested in. However, I'm not sure why that would be better - it would accomplish the exact same thing in twice the amount of time. The tests pass before, and they will pass after as well.
Ultimately this patch isn't what I'm interested in anyway - I'm looking at new tests (already written), which expose quite a few new bugs. I just need to figure out a refcounting issue before my test will terminate properly. Your approach is more appropriate from an observer perspective, but forces me to work on breaking down patches, instead of writing new functionality.
==========
Were those patches dropped for a similar reason ?
[WINED3D] Pbuffer/ActiveRender cleanup #2 [WINED3D] Clean up some shader issues
On 26/09/06, Ivan Gyurdiev ivg231@gmail.com wrote:
change I'm interested in. However, I'm not sure why that would be better
- it would accomplish the exact same thing in twice the amount of time.
Well, there are two things I can think of right away: - It makes thing easier to review - It helps with tracking down regressions
H. Verbeet wrote:
On 26/09/06, Ivan Gyurdiev ivg231@gmail.com wrote:
change I'm interested in. However, I'm not sure why that would be better
- it would accomplish the exact same thing in twice the amount of time.
Well, there are two things I can think of right away:
- It makes thing easier to review
- It helps with tracking down regressions
I will make the required changes, as discussed with julliard on IRC.
...but this is really a theoretical argument. This is a framework mostly written by me to begin with, so the chances of breaking it are essentially zero. If it does get broken, the damage factor will be very low - it won't test as well as it used to.
If you want to help prevent regressions, please help write more state tests using this framework, rather than focusing on unimportant details. We need more test coverage! In fact, part of the motivation behind the changes being made is to make the framework a bit easier to understand and more flexible, so that others can contribute. If you don't like this framework, feel free to redesign it - but we have a number of obvious bugs in state management that need fixing, and test verification is the first step to doing that.
Ivan
On 26/09/06, Ivan Gyurdiev ivg231@gmail.com wrote:
...but this is really a theoretical argument. This is a framework mostly written by me to begin with, so the chances of breaking it are essentially zero. If it does get broken, the damage factor will be very low - it won't test as well as it used to.
In this particular case, sure. I was talking more about the general case.
-
Alexandre Julliard -
H. Verbeet -
Ivan Gyurdiev