Attached to this email is a rough implementation of the main GDI+ containers functions. It performs correctly in a few simple tests I ran on it, but it's not quite ready to send off to wine-patches. I'd like to get some feedback here, since I'm certain that I'm doing some things wrong. I'm wondering about two things specifically. Since the vast majority of my programming experience has been with object oriented languages, I'm not used to how to do things in C.
First, where should the utility functions and variables go? Right now, they're just thrown in above the functions I'm working on. It seems messy to me to have, for example, low and high (which need to be renamed) declared where they are. Do they belong there (or, rather, near the top of graphics.c like some other utility functions), or should they be placed elsewhere?
Second, I implemented a simple linked list to manage the stack of GpGraphics objects. I would imagine that there's some standard way of doing stacks, rather than creating a simple one with utility functions as I have here. Can someone point me in the direction of the proper way to do stacks like this in C?
Finally, while the code's still rough and needs cleaning up, correctness checking, using Windows types and const properly, etc., am I doing anything glaringly wrong? As I mentioned, it works as expected in the tests I've done, so I can't be too far off the mark :)
Thanks for taking a look, Andrew
Patch attached. Also available at: http://www.brightnightgames.com/wine/gdipcontainers.patch
--- dlls/gdiplus/graphics.c | 125 ++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 119 insertions(+), 6 deletions(-)
On Tue, Jun 30, 2009 at 11:48 PM, Andrew Eikumandrew@brightnightgames.com wrote:
Attached to this email is a rough implementation of the main GDI+ containers functions. It performs correctly in a few simple tests I ran on it, but it's not quite ready to send off to wine-patches. I'd like to get some feedback here, since I'm certain that I'm doing some things wrong. I'm wondering about two things specifically. Since the vast majority of my programming experience has been with object oriented languages, I'm not used to how to do things in C.
First, where should the utility functions and variables go? Right now, they're just thrown in above the functions I'm working on. It seems messy to me to have, for example, low and high (which need to be renamed) declared where they are. Do they belong there (or, rather, near the top of graphics.c like some other utility functions), or should they be placed elsewhere?
Second, I implemented a simple linked list to manage the stack of GpGraphics objects. I would imagine that there's some standard way of doing stacks, rather than creating a simple one with utility functions as I have here. Can someone point me in the direction of the proper way to do stacks like this in C?
A quick answer to this question: In C - no; in wine source - yes. http://source.winehq.org/source/include/wine/list.h#L30 . There's also a rb tree somewhere.
Finally, while the code's still rough and needs cleaning up, correctness checking, using Windows types and const properly, etc., am I doing anything glaringly wrong? As I mentioned, it works as expected in the tests I've done, so I can't be too far off the mark :)
Thanks for taking a look, Andrew
Patch attached. Also available at: http://www.brightnightgames.com/wine/gdipcontainers.patch
dlls/gdiplus/graphics.c | 125 ++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 119 insertions(+), 6 deletions(-)
Mike.
Comments that become irrelevant when you do this right are helpfully enclosed in (parentheses).
First, where should the utility functions and variables go? Right now, they're just thrown in above the functions I'm working on. It seems messy to me to have, for example, low and high (which need to be renamed) declared where they are. Do they belong there (or, rather, near the top of graphics.c like some other utility functions), or should they be placed elsewhere?
If a symbol is only used within a single .c file and not exported in the .spec, you should make it a static function. Other than that, you seem to be declaring functions fine.
+GraphicsNode* first_graphics_node = NULL;
This should be stored in the GpGraphics struct. Each Graphics object should have its own independent stack.
(The way you use it is also a little backwards. Adding to the beginning of a linked list is an O(1) operation, and adding to the end is O(n). You could use better algorithms if you reverse the order of the list. But don't worry about that too much. wine/list.h is doubly-linked so working on either end is O(1).)
- dst->worldtrans = GdipAlloc(sizeof(GpMatrix));
Don't do this. Use standard functions to create objects. Just trust me, it's better that way. Or read the parenthesized comments if you don't.
- high -= 1;
- low += 1;
- *state = (high << 16) + low;
I have no idea what you're trying to accomplish by doing this. If you got that by inspecting the values you get from native, I think you're watching native a little too closely.
+void delete_graphics(GpGraphics* graphics){
- GdipFree(graphics->worldtrans);
- GdipFree(graphics->clip);
- GdipFree(graphics);
+}
This is a bad idea. You're defining an additional kind of graphics object that follows different rules and must be created and destroyed differently. That's confusing.
Instead of copying the entire graphics struct, you should explicitly save the fields you need.
(You shouldn't be freeing graphics, since it's a member of a struct and not a heap-allocated thing whenever delete_object is called.)
(memcpy() of regions is not sound. A region object can refer to any number of heap-allocated "region elements" that you'll end up sharing with an existing region. The existing region will then likely free them if it's modified in the meantime. Your copy will then refer to freed memory and the program will go to hell.)
(If you copy_graphics TO a real Graphics object, as you do in pop_graphics_node, you leak the object's current transformation matrix and clipping region.)
Vincent Povirk
-
Andrew Eikum -
Mike Kaplinskiy -
Vincent Povirk