Re: svn commit: r23239 - trunk/subversion/libsvn_subr

Malcolm,

thanks for the review!

I've fixed most of your remarks in r23278, r23292-4 and r23330. Some
remarks follow:

Malcolm Rowe wrote:

>> + write_to_file(log_file,
>> + "eax=%08x ebx=%08x ecx=%08x edx=%08x esi=%08x edi=%08x\n",
>> + context->Eax, context->Ebx, context->Ecx,
>> + context->Edx, context->Esi, context->Edi);
>
> What happens with x64 or ia64 executables, incidentally?

Apparently the code doesn't build on x64. I'd rather leave this to fix
by someone with a 64-bit OS available to test. r23330 adds an #error
directive which stops the build on non-x86 platforms.

> Are you intending for this code to work when compiled with UNICODE
> defined? If yes, you just called CreateFileW with an ANSI filename. If
> no, why do you call GetTempPathA explicitly? (There are also other
> instances of this elsewhere, references to TCHAR etc).

Not really no, I fixed those instances which I found, there were only
two, did you see more?

>
> Finally, is it okay to assume that all command-line programs will be
> happy for us to add in our own exception handler? Seems a bit rude for
> a library to just assume that, though I doubt there's any real harm.

You mean applications that wrap svn.exe? Since the current behavior is
to hang the process with a dialog I don't presume it matters much.
Further I assume only the svn* executables to invoke svn_cmdline_init,
is that correct?

There's also the SVN_USE_WIN32_CRASHHANDLER define to disable building
the crash handler altogether.

Lieven

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Feb 2 20:54:31 2007