Dmitry,
On 11/21/2013 11:05 AM, Dmitry Timoshkov wrote:
Alexandre Julliard julliard@winehq.org wrote:
Vincent asked you to support more cases, you refused to do that. Then he asked you to at least add FIXMEs for the cases you don't want to support, and you refused to do that too. I wouldn't call that addressing his concerns.
First of all I pointed out that it's most likely impossible to even encounter the cases of unsupported flags since GdipCreateRegionRgnData is supposed to handle the data produced by GdipGetRegionData on the same implementation. Basically any case of a failing GdipCreateRegionRgnData
so an ERR() or even an assert() would be more appropriate?
call should be investigated separately and the reasons may be different and legitimate. Adding FIXMEs for every GdipCreateRegionRgnData failure seems not worth the trouble at this point IMO since it's the very first implementation of the data parser.
IMHO quite the contrary. On the first implementation you try to be as verbose as possible to not hide bugs and make it easier to detect the issues. You know the code so for you it doesn't matter, but it can make all the difference for everybody else.
Next, my reaction to Vincent's comments was influenced by the fact that the patch was already rejected under shaky grounds, so arguing about
The initial reject email made perfectly sense to me. Identifiers with a leading underscore have a special meaning: "All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces." from http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf chapter 7.1.3 "Reserved identifiers", paragraph 1. From that the common C practice evolved to use them to "hide" the tag name. And your code is not hiding it but using it as a normal struct definition. So somebody more versed in the abyss of the C standard will get a WAT? moment when seeing the code.
After all it is *not a problem* at all to miss such subtleties, happens in the heat of the battle and C has a few pitfalls. Nobody is perfect and that's why we have code review. And if you still disagree please use technical reasons, but don't give us this hurt feelings bullshit.
In this case you could construct a "taste matter" out of it, but for that you wasted way too much time. Choose your battles wisely. That's what I do in those matter of taste situations: curse at Alexandre (optional, sometimes just shrugging), change the patch, move on.
As I'm being asked regularly, even by Wine developers that have been around some time here is an important thing to keep in mind:
Wine is being optimized for the human reader. For a good while now. That is the most scarce and limiting resource.
So small things like following the principle of the least surprise do matter as they lower the barrier of entry. Consistency matters too. And *no*, as a Wine developer that has been around for a while you *cannot* use the excuse "but I just moved/copied existing code". Except in a sentence like "Oops, sorry! Missed it, I was "betriebsblind" (German for becoming blind to the issue due to being too entrenched in the subject matter).
bye michael