Dscho, Thanks a lot for the reviews!
If the next time, you prefer one answer to three patch reviews, please, let me know. For now, two others will follow. Big question first: can we use some parts of git? For example, git-compat-util.h? Or regex.[ch]? Smaller questions below. > -----Original Message----- > From: Johannes Schindelin [mailto:[EMAIL PROTECTED] > Sent: Sunday, February 03, 2008 6:16 PM > > diff --git a/dll.c b/dll.c > > index bab9d9a..40f52ae 100644 > > --- a/dll.c > > +++ b/dll.c > > + static char module_filename[MAX_PATH] = { '\0' }; > AFAIK static variables are initialised all-zero anyway. It seems to me the code that explicitly initializes [all] variables is easier to read. As a side note, IIRC it depends on (and/or configurable in) the compiler. So, unless you insist, I'd like to keep explicit initialization. > > + /* TODO: find an elegant way to pass the installation type > > + down here */ > Why not a global variable? Probably because I did not find global variables for this kind of use elegant enough :) But given that it's the "last resort" thing, you are probably right. > > +/* replaces a substring pattern with a string > > +static char *strreplace(char *string, > > + const char *pattern, > > + const char *replacement) > This is not safe <snip> Oh! I was really hoping for something like "well, well... lack of Unix experience shows... why not use regex library" :) This function alone is the reason for the big question. > > +static const REG_VALUE registry_info[] = { > > + { "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Shell > > Extensions\\Approved", "@@CLSID@@", "@@PROGRAM_NAME@@" }, > How about > #define SHELLEXT "SOFTWARE\\...\\Shell Extensions" > static const REG_VALUE registry_info[] = { > { SHELLEXT "\\Approved", "@@CLSID@@", "@@PROGRAM_NAME@@" }, I'm afraid I don't fully understand this comment. Do you mean #define repeated parts? If so, would you prefer to see: #define CLASSESROOT "Software\\Classess\\" #define CONTEXTMENUHANDLER "shellex\\ContextMenuHandlers\\@@PROGRAM_NAME@@" static const reg_value registry_info[] = { { CLASSESROOT "Drive\\" CONTEXTMENUHANDLER, NULL, "@@CLSID@@" } In other words, how small the part needs to be to warrant a define? > Also, please use a lower-case name for reg_value (upper-case in the C > world is commonly used for #define'd stuff). Just to satisfy my curiocity: have I mistaken Windows' way of defining types, structures, enums for "common in C"? In other words, isn't it common to use REG_VALUE as a __type__ (I do understand that a variable would usually be reg_value). > > +static char *get_registry_path(const char *src, char > > + dst[MAX_REGISTRY_PATH]) > > +{ > Since this function is not called in multi-threaded manner, why not > just having a "static char dst[MAX_PATH]"? I'm afraid I don't understand. The reason to pass the destination buffer as a parameter is because I need to hold three different dst at the same time (see the use case in create_reg_entries). So, I don't see how static char dst[MAX_PATH] will work. > > +HRESULT PASCAL DllInstall(BOOL bInstall, LPCWSTR pszCmdLine) > > { > > - if (reason == DLL_PROCESS_ATTACH) { > > - object_count = lock_count = 0; > > - DisableThreadLibraryCalls(instance); > > + BOOL bMachine = NULL != wcsstr(pszCmdLine, L"machine"); > That is not enough, right? > (just imagine somebody happens to call "regsrv32 > C:/machine/git-cheetah.dll"). At first, you do realize that "if (reason...)" is from completely different function, don't you? I just want to make it clear that the patch is not obvious because I haven't really changed DllMain, I just added bunch of code to the rest of the file. And... I had to re-format-patch 3 times because could not understand it :) At second, the implementation of DllInstall seems enough (with a caveat, described in its comments about debug-only machine-wide). The reason is: regsvr32 has [seemingly] complicated way to identify what to call. Here is a simplified version: (no keys) = DllRegister -i = DllRegister + DllInstall -n -i = DllInstall So, your example will call DllRegister, which currently performs a normal registration for the current user. To perform machine registration only - "regsvr32 -n -i:machine git-cheetah.dll". Or I misunderstood the comment. -- Kirill.
