Hi Michael,
Michael Stefaniuc mstefani@redhat.com 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?
In which places? I asked Vincent to add them if he thinks that he knows such places and such cases.
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.
I did my best and covered my implementation with a test for every single thing that current GdipGetRegionData implementation generates, and explained why it's unreasonable to expect unknown data as input.
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.
Just do grep in dlls/*.c for structures with underscores in their names and try to explain what so special about my patch, and why existing code has so much structure definitions with underscores. Did I miss something and the rules have changed or it's a new one invented just before my patch has arrived?
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.
I've already said, that if the underscres would be silently removed when the patch being committed I'd just skipped an moved on, but the patch was rejected with later public accusions about defensive replies. That of course were the pure technical reasons.