That's is very interesting and useful, but I think there's two things
that
can be improved: first the debug delay patch, I wasn't able to find it out searching with gmane, maybe if you can add a link to the post on gmane it should be better :)
Yeah, it's a bit of a pain keeping track of useful patches that were never merged. Maybe we need a list somewhere.
The second thing is about the coding style. As I said before, I have a patch for shlwapi which fixes some MSXML/Borland XML Components problems with the builtin one, which wasn't accepted for some problem about the style and the placement of the variables. I posted it as a bug at http://bugs.winehq.org/show_bug.cgi?id=2385 . If you can add something about the coding style I can fix it up and repost it for inclusion.
A few things that jump out at me (not really reviewed the code itself):
- No C++ // style comments, I'm afraid. They aren't portable enough. - Use HeapAlloc() over malloc(), as it says in the cheatsheet - Are you sure there is a debugstr_a?? I can't think what it would do, ANSI strings can just be printed by regular printf - You have to declare all the variables at the start of a block. Yes, it's a pain. Another C portability rule. So, can't do this:
LPSTR foo = bar; StrCpyA(a, b); int i;
They all have to go at the top.
- Minor stylistic note: you can use strcpy rather than StrCpyA. But I doubt this was a problem. - Usually best to space stuff out, ie rather than this:
if ( temp_out == temp_out2 ) { temp_in += 3; continue; }
use this:
if ( temp_out == temp_out2 ) { temp_in += 3; continue; }
... again I doubt Alexandre would reject the patch because of this.
- //if ( *temp_in == '.' ) ... wine code generally doesn't do this. If you find it hard to match up a closing brace with the statement that opened it maybe you need ... *fanfare* EMACS! :)
- Is it possible to implement the ANSI version by converting to unicode then calling the wide version, rather than duplicate it?
The rest looks fine, I expect it was just the C portability stuff that tripped you up. Please do try again!
thanks -mike
"Mike" == Mike Hearn m.hearn@signal.QinetiQ.com writes:
...
Mike> cheatsheet - Are you sure there is a debugstr_a?? I can't think Mike> what it would do, ANSI strings can just be printed by regular Mike> printf ...
hertz:/spare/bon/wine-realclean/wine> grep debugstr_a include/*/* include/wine/debug.h:inline static const char *debugstr_an( ... include/wine/debug.h:inline static const char *debugstr_a( ...
The keep wine from crashing on invalid strings and do some additional formating.
Bye
Mike Hearn wrote:
The second thing is about the coding style. As I said before, I have a patch for shlwapi which fixes some MSXML/Borland XML Components problems with the builtin one, which wasn't accepted for some problem about the style and the placement of the variables. I posted it as a bug at http://bugs.winehq.org/show_bug.cgi?id=2385 . If you can add something about the coding style I can fix it up and repost it for inclusion.
A few things that jump out at me (not really reviewed the code itself):
- No C++ // style comments, I'm afraid. They aren't portable enough.
Yes, as Mike says we don't allow these because they are C99 only and we don't like making Wine unnecessarily unportable across compilers.
- Use HeapAlloc() over malloc(), as it says in the cheatsheet
- Are you sure there is a debugstr_a?? I can't think what it would do, ANSI strings can just be printed by regular printf
Yes. It is useful for printing large strings that would otherwise overflow the static buffer and for strings that contain non-ASCII characters.
You have to declare all the variables at the start of a block. Yes, it's a pain. Another C portability rule. So, can't do this:
LPSTR foo = bar; StrCpyA(a, b); int i;
They all have to go at the top.
I also finder easier to read functions that have all the variables declared at the top of each block. Don't know the type of a variable? Then scan the top of each block - no need to look at any lines in between.
Minor stylistic note: you can use strcpy rather than StrCpyA. But I doubt this was a problem.
Usually best to space stuff out, ie rather than this:
if ( temp_out == temp_out2 ) { temp_in += 3; continue; }
use this:
if ( temp_out == temp_out2 ) { temp_in += 3; continue; }
... again I doubt Alexandre would reject the patch because of this.
//if ( *temp_in == '.' ) ... wine code generally doesn't do this. If you find it hard to match up a closing brace with the statement that opened it maybe you need ... *fanfare* EMACS! :)
Heh, this is fairly similar to Alexandre's style so be careful :) Although his is more like: if (cond) func( args );
- Is it possible to implement the ANSI version by converting to unicode then calling the wide version, rather than duplicate it?
Most of the time it is worth it, but probably not in this case and for other string processing functions. The bad thing is that the A and W functions could get out of sync, so it is best to make sure they work perfectly (and don't have any fixme's) before the patch is accepted.
One additional niggle: temp_in and temp_out aren't terribly good variable names.
Rob
Yes. It is useful for printing large strings that would otherwise overflow the static buffer and for strings that contain non-ASCII characters.
I stand corrected (thanks to Uwe as well).
I also finder easier to read functions that have all the variables declared at the top of each block. Don't know the type of a variable? Then scan the top of each block - no need to look at any lines in between.
Ah, I use the semantic package for emacs to help with this. If you leave the cursor on a variable for a few moments the declaration appears in the echo area. Very nice.
I still tend to prefer having variables declared closest to where they're used, but you could equally say that if a function is so big the distance is really causing problems it probably should be split up and made smaller.
One additional niggle: temp_in and temp_out aren't terribly good variable names.
Indeed. I try and keep variables and functions to one word, where possible. Obviously clarity is more important though ...
Mike Hearn wrote:
Yeah, it's a bit of a pain keeping track of useful patches that were never merged. Maybe we need a list somewhere.
Yes, it can be very helpful, if I can be helpful in any way, I'm here :)
A few things that jump out at me (not really reviewed the code itself):
Thanks a lot, I'll review the patch tonight and I'll resubmit it after I cleaned it up :)
I have only a doubt:
- Is it possible to implement the ANSI version by converting to unicode then calling the wide version, rather than duplicate it?
How can I convert an ansi string to a unicode one and reverse? So I can redo the patch with the calls :)
Thanks,
-
Diego 'Flameeyes' Pettenò -
Mike Hearn -
Robert Shearman -
Uwe Bonnes