gdiplus: Implement GdipCreateRegionRgnData. Take 2.

Dmitry Timoshkov dmitry at baikal.ru
Thu Nov 21 08:46:26 CST 2013


Hi Michael,

Michael Stefaniuc <mstefani at 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.

-- 
Dmitry.



More information about the wine-devel mailing list