gdiplus: Implement GdipCreateRegionRgnData. Take 2.

[Michael Stefaniuc]

Dmitry,

On 11/21/2013 11:05 AM, Dmitry Timoshkov wrote:
> Alexandre Julliard <julliard at 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