2016-03-08 0:22 GMT+01:00 Paul Gofman gofmanp@gmail.com:
On 03/08/2016 01:42 AM, Matteo Bruni wrote:
a proper implementation would be even better (although it may be non obvious to find out if a parameter is used, depending on what "used" really means - it needs tests)...
You always need tests, but a reasonable guess would be whether the parameter is referenced by the technique.
That's likely to be the case, but yeah - tests.
I actually already got real implementation for this function (here inside a big & dirty cumulative patch https://bugs.winehq.org/attachment.cgi?id=53869), but this requires testing as you say. My motivation to change the stub at this point was that it currently preventing testing the other bigger parts of effect framework on real applications, and real implementation will be easier test and add later. Since I can't add alltogether, I need to choose what to deal with first, and I thought it might be easier to verify a bigger piece when I get them working in real life rather then putting all the blocks in before getting anything real working.
I get your point, that's why I signed off your patch :)
Henri's point was also valid though. The other option was to not include this patch upstream (keeping this patch or your work-in-progress implementation in your private tree for testing purposes) and only submit the final IsParameterUsed() implementation when it's ready. In the end this means I'm trusting you to submit a proper implementation with tests later on.