Dmitry Timoshkov dmitry@baikal.ru writes:
This version of the patch follows Windows behaviour by refusing to create an empty path in a region, and marks the test as broken when some Windows versions fail to properly clear an aligned DWORD.
dlls/gdiplus/region.c | 219 ++++++++++++++++++++++++++++++++++++++++---- dlls/gdiplus/tests/region.c | 65 +++++++++++++ 2 files changed, 266 insertions(+), 18 deletions(-)
diff --git a/dlls/gdiplus/region.c b/dlls/gdiplus/region.c index 832ce69..bc625f5 100644 --- a/dlls/gdiplus/region.c +++ b/dlls/gdiplus/region.c @@ -76,6 +76,28 @@ WINE_DEFAULT_DEBUG_CHANNEL(gdiplus); #define FLAGS_NOFLAGS 0x0 #define FLAGS_INTPATH 0x4000
+struct _memory_buffer +{
- const BYTE *buffer;
- INT size, pos;
+};
+struct _region_header +{
- DWORD size;
- DWORD checksum;
- DWORD magic;
- DWORD num_children;
+};
+struct _path_header +{
- DWORD size;
- DWORD magic;
- DWORD count;
- DWORD flags;
+};
There's no need to add leading underscores, it's obvious that these are not Windows types.
Alexandre Julliard julliard@winehq.org wrote:
There's no need to add leading underscores, it's obvious that these are not Windows types.
If that's the only reason of marking this patch as rejected then I don't see much point. That's my code and it should be up to me what style of defining structures I have to use.
Dmitry Timoshkov dmitry@baikal.ru wrote:
Alexandre Julliard julliard@winehq.org wrote:
There's no need to add leading underscores, it's obvious that these are not Windows types.
If that's the only reason of marking this patch as rejected then I don't see much point. That's my code and it should be up to me what style of defining structures I have to use.
I should add that this kind of a not justified rejection easily kills any motivation to send patches at all.
I hoped that this is some kind of a joke or a test, and I simply didn't get it, but looks like my hope was futile. It appears that just moving existing structure definition (yes, they all exist in current code) to the beginning of the file (so that it could be used in more places) is forbidden without any real explanation. That's too much even for a person like me with 14 years history of working on Wine, I can imaging what new-comer feels about such a reject, and there should be not wonders why he/she would go away.
Alexandre, if you would silently remove those underscores if you really don't like to see them I'd just probably decided not bother to comment once I saw it in the commit, but plain rejection of the patch just because of that looks at least strange and unexplainable.
Please reconsider or give a better explanation.
Thanks.
Dmitry Timoshkov dmitry@baikal.ru writes:
I hoped that this is some kind of a joke or a test, and I simply didn't get it, but looks like my hope was futile. It appears that just moving existing structure definition (yes, they all exist in current code) to the beginning of the file (so that it could be used in more places) is forbidden without any real explanation. That's too much even for a person like me with 14 years history of working on Wine, I can imaging what new-comer feels about such a reject, and there should be not wonders why he/she would go away.
The structs were not used at all previously, only the variables. Now that they are used as structs they should have decent names.
Alexandre, if you would silently remove those underscores if you really don't like to see them I'd just probably decided not bother to comment once I saw it in the commit, but plain rejection of the patch just because of that looks at least strange and unexplainable.
You also received comments from Vincent that you need to address.
I should add that this kind of a not justified rejection easily kills any motivation to send patches at all.
This goes both ways: every time someone comments on a patch of yours, you reply defensively and state that you refuse to make the requested changes. That doesn't exactly encourage giving you good feedback.
Alexandre Julliard julliard@winehq.org wrote:
I hoped that this is some kind of a joke or a test, and I simply didn't get it, but looks like my hope was futile. It appears that just moving existing structure definition (yes, they all exist in current code) to the beginning of the file (so that it could be used in more places) is forbidden without any real explanation. That's too much even for a person like me with 14 years history of working on Wine, I can imaging what new-comer feels about such a reject, and there should be not wonders why he/she would go away.
The structs were not used at all previously, only the variables. Now that they are used as structs they should have decent names.
I don't recall any rule for structure naming that everyone working on Wine should follow. Simple grep in dlls/*.c source shows quite a bit of structures with underscores in their names. How is my patch different from existing ones? What makes an underscore in the private structure name unacceptable? Is that just a personal preference or something else?
Alexandre, if you would silently remove those underscores if you really don't like to see them I'd just probably decided not bother to comment once I saw it in the commit, but plain rejection of the patch just because of that looks at least strange and unexplainable.
You also received comments from Vincent that you need to address.
I think that I've answered to Vincent's concerns, besides they have nothing to do with the reasons this patch has been rejected, Vincent sent his comments much later after your e-mail. If you think that I didn't address some of Vincent's questions please ask for clarifications in appropriate thread.
I should add that this kind of a not justified rejection easily kills any motivation to send patches at all.
This goes both ways: every time someone comments on a patch of yours, you reply defensively and state that you refuse to make the requested changes. That doesn't exactly encourage giving you good feedback.
I don't think that my replies are that bad in general, and definitely not every time. I'm always trying to do my best regarding technical side of the things.
I think that a person spending his/her own time (and often without any payment at all) deserves slightly more respect regarding his/her work. Just try sometimes pretend that you are one of us pure people who sends a patch to somebody else and completely depends on that person's decision, but instead he/she sees "I don't give good feedback, you are too defensive about your patches". It's called "persistense", and without it it's often impossible to get any patch committed in this project.
Dmitry Timoshkov dmitry@baikal.ru writes:
Alexandre Julliard julliard@winehq.org wrote:
I hoped that this is some kind of a joke or a test, and I simply didn't get it, but looks like my hope was futile. It appears that just moving existing structure definition (yes, they all exist in current code) to the beginning of the file (so that it could be used in more places) is forbidden without any real explanation. That's too much even for a person like me with 14 years history of working on Wine, I can imaging what new-comer feels about such a reject, and there should be not wonders why he/she would go away.
The structs were not used at all previously, only the variables. Now that they are used as structs they should have decent names.
I don't recall any rule for structure naming that everyone working on Wine should follow. Simple grep in dlls/*.c source shows quite a bit of structures with underscores in their names. How is my patch different from existing ones? What makes an underscore in the private structure name unacceptable? Is that just a personal preference or something else?
A leading underscore is traditionally used to denote symbols that are somehow private or outside of the normal namespace. Basically it's for places where you'd want an anonymous object, but the syntax requires a name. For instance a struct that's only used for a typedef, or a function that's not meant to be called, or a variable that's a placeholder but never referenced, things like that. It's a standard C convention, and adhering to such conventions makes code easier to understand for everybody.
Alexandre, if you would silently remove those underscores if you really don't like to see them I'd just probably decided not bother to comment once I saw it in the commit, but plain rejection of the patch just because of that looks at least strange and unexplainable.
You also received comments from Vincent that you need to address.
I think that I've answered to Vincent's concerns, besides they have nothing to do with the reasons this patch has been rejected, Vincent sent his comments much later after your e-mail. If you think that I didn't address some of Vincent's questions please ask for clarifications in appropriate thread.
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.
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 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.
Next, my reaction to Vincent's comments was influenced by the fact that the patch was already rejected under shaky grounds, so arguing about adding FIXMEs (actually jus one FIXME about path flags) didn't change the fact that the patch is already rejected, and even adding the proposed FIXME wouldn't change anything.
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
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.
Dmitry,
I agree about the underscores. It seems like a very minor point, and I personally wouldn't have expected it to be a problem.
From my perspective, it seems like adding FIXME's should be easy,
they'd be potentially helpful for diagnosing problems, and there doesn't seem to be a drawback to adding them (either they're never hit and have no effect, or they are hit and they indicate a potential problem). I really don't understand your objection to adding them, and that is what makes your response seem "defensive" to me at least. But yes, if your patch is accepted without FIXME's I will send a patch to add them myself when I have time.
I really don't feel like this is anything new. If you're a Wine developer, you will sometimes have to make changes you disagree with or think are unnecessary to get a patch in. Sometimes this is to address very minor points. You will also have to decide which feedback is really important and which you can ignore or argue with, and then you have to update your model when it turns out Alexandre doesn't see things the way you thought he would. I don't know if this is a good thing. Maybe it means we're losing some good contributions due to people getting frustrated by the process and giving up. But I've found this to be the case for at least as long as I've been a Wine contributor, and since you've been around longer than I have, I'd expect you to be used to it by now.
Vincent Povirk madewokherd@gmail.com wrote:
From my perspective, it seems like adding FIXME's should be easy,
they'd be potentially helpful for diagnosing problems, and there doesn't seem to be a drawback to adding them (either they're never hit and have no effect, or they are hit and they indicate a potential problem). I really don't understand your objection to adding them, and that is what makes your response seem "defensive" to me at least. But yes, if your patch is accepted without FIXME's I will send a patch to add them myself when I have time.
Adding a FIXME is not a propblem at all, as I already mentioned there was no point in talking about a FIXME since the patch has been already rejected. Please find this snippet in one of my replies: "If you see obvious places of adding such FIXMEs then send a patch which adds them, personally I don't care at this point about it."
I really don't feel like this is anything new. If you're a Wine developer, you will sometimes have to make changes you disagree with or think are unnecessary to get a patch in. Sometimes this is to address very minor points. You will also have to decide which feedback is really important and which you can ignore or argue with, and then you have to update your model when it turns out Alexandre doesn't see things the way you thought he would. I don't know if this is a good thing. Maybe it means we're losing some good contributions due to people getting frustrated by the process and giving up. But I've found this to be the case for at least as long as I've been a Wine contributor, and since you've been around longer than I have, I'd expect you to be used to it by now.
Yes, I'm (mostly) used to change my patches regardless of how I feel about the change, but there are limits after which it gets ridiculous, not justified and even a personal insult. I really don't understand why it has happened to my patch while there are lots of similar cases already, and why the underscores simply were not removed at the commit time. Being a long time contributor doesn't mean that I can explain or understand the reasons behind this particular case, but this kind of thing is able to completely kill any motivation of further work.
-
Alexandre Julliard -
Dmitry Timoshkov -
Michael Stefaniuc -
Vincent Povirk