Cursor & Icon patches
hverbeet at gmail.com
Mon Aug 3 05:25:36 CDT 2009
I only looked at the first couple of patches, but this probably needs
more work. E.g. in the first patch you white space changes and changes
to apparently unrelated code. It also helps to be a bit more
descriptive about what you're testing, instead of something like "Add
more tests for SetCursor & DestroyCursor".
- Patch 2: I'm not sure we need the extra traces there, since +relay
will show what's called, and you can always add more specific traces
while debugging. Regardless, you also have white space changes in this
- Patch 3 & 4: We very rarely care about LastError after successful
calls. Pretty much the only exception is if a (real world) application
depends on the behaviour. So you'll need at least tests and an
application that depends on this for those patches to go in.
- Patch 5: Again, white space / formatting.
- I think some of the comments as well as some of the asserts are a
- The forward declaration for get_bitmap_width_bytes() is redundant.
- cursor_rb_functions and cursor16_rb_functions should probably be static.
- CURSORICON_init() should fail if initializing the lookup trees
fails (and as a result, process_attach() as well).
- Although it will probably work, I think it's ugly to pass the key
to wine_rb_get() as the pointer instead of passing a pointer to the
key as intended.
- wine_rb_put() might fail.
- Using map_entry as handle in add_cursor_entry() is unsafe on
64-bit. I also think it's somewhat questionable that
add_cursor_entry() creates the 32-bit handle in the first place. The
way it should work is that create_cursor creates a cursor handle from
the data, and an entry in the lookup tree is only created when no
16-bit cursor exists yet in get_cursor_handle16(). In general, it
looks like you're abusing the 16 <=> 32 lookup trees to determine if a
handle is valid. You really need a handle table for that, either in
the server, or something local to user32.
More information about the wine-devel