At 08:17 PM 25/07/2005 +0200, SteveKing steveking-at-gmx.ch |subversion dev
list| wrote:
>Ok, I guess this will be my last try:
Never give up! Never surrender!
>I've changed the crash reporting dll so it can save the zipped report
>automatically and print a message with the location to where it was
>saved to stderr.
>Also, I've added a function to enable/disable the UI part after the dll
>was loaded. That way, the svn client could enable the UI part later on
>when the command line params are read (I don't know enough to add such a
>param myself, I have to leave that to someone else).
>
>By default, all svn apps now use the crash reporter in console mode,
>i.e. without popping up a dialog but just saving the report and print
>info about it to the console stderr.
This is all pretty nifty :-) I notice, however, that the patch isn't quite
perfect yet, and I'm not talking about code style :-)
[snip]
>+typedef BOOL (__stdcall *CrashGetLogFile) (void * crashstate);
^-- Good, but would be even better as "WINAPI" for those obscure systems
that nobody ever uses any more that don't know what an __stdcall is.
>+typedef void * (*InstallEx)(CrashGetLogFile func, const char * mailto,
[..]
>+typedef void (*UninstallEx)(void * crashstate);
[..]
>+typedef void (*EnableUI)(void);
[..]
>+typedef void (*DisableUI)(void);
^-- Not quite as good. These functions are all from the same DLL as the
first function, no? They probably should also have WINAPIs. I mean, the
code works without them, but only because these typedefs are defaulting to
__stdcall, since it is the default calling convention's default. Suppose
someone decided to, for no reason discernible to us now, add /Gd onto the
compile command-line for the subversion command-line tool. Then, the
compiler would, as far as I know, assume that these function pointers all
pointed at __cdecl functions, and very odd things would happen. The crash
reporter would crash, resulting in mass confusion & hysteria!
>+ FreeLibrary(crashDll);
>+ crashDll = 0;
^-- What happens if this code runs twice by accident and you end up passing
NULL into FreeLibrary? I'm not implying anything; I honestly don't know.
For all I know, it'll just quietly return an error, but it might also
segfault. I don't think the function is intended to be used on NULL
pointers...
In general, I think it's safer to avoid assuming any state when you aren't
in control of the code that is calling into your module.
I think you've done a wonderful job of putting up with the style nazis that
invariably lurk around any large open source project, like trolls waiting
under the bridge to gobble up unwary travelers. I have encountered style
issues on other projects as well, such as the mono project, and through my
experience have learned that while the project's style is invariably
horrendous & far uglier than what you use yourself, you can save yourself a
lot of grief with a relatively small amount of research. The 1800+ lines in
HACKING aren't all about code style, and they _are_ indexed, if not with
exact page or line numbers to go to. I read through HACKING in about 20
minutes. For version 1.2.0, "Coding Style" starts on line 231 ends on line
275, a mere 44 lines of style guidelines!
For what it's worth, my own personal style is fairly similar to the SVN
convention. I don't indent braces themselves, and I don't put spaces before
parentheses in function calls, but I do abhor tabs in much the same way :-)
Jonathan Gilbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 27 07:27:17 2005