Re: imagehlp: Implement (parts of) BindImageEx

[Andrew Eikum]

I don't know this area of Wine, but some more general thoughts below.

On Tue, Aug 12, 2014 at 05:34:42PM +0200, Bernhard Reiter wrote:
> +void * WINAPI ImageDirectoryEntryToDataEx( void *base, BOOL image, unsigned short dir,
> -
>                                       unsigned long *size, PIMAGE_SECTION_HEADER *section );

Is there a reason for forward-declaring this instead of including
dbghelp.h?
> */
>  BOOL WINAPI BindImageEx(
> - DWORD Flags, PCSTR ImageName, PCSTR DllPath, PCSTR SymbolPath,
> - DWORD Flags, const char *ImageName, const char *DllPath, const char *SymbolPath,
> PIMAGEHLP_STATUS_ROUTINE StatusRoutine)
>
> {
> - FIXME("(%d, %s, %s, %s, %p): stub\n",
> - LOADED_IMAGE loaded_image;
> - const IMAGE_IMPORT_DESCRIPTOR *import_desc;
> - unsigned long size;
> - char dll_fullname[MAX_PATH];
> -
> - FIXME("(%d, %s, %s, %s, %p): semi-stub\n",

I know it's inconsistent with the rest of the file, but I'd use
four-space indents for this function, since it's all new code and you
have the opportunity to fix it. Not a big deal if you'd rather keep
two spaces.
> - (*StatusRoutine)( BindImportModule, ImageName, dll_name, 0, 0);
> -

Does Windows really crash when given NULL StatusRoutine?
> - thunk = ImageRvaToVa( loaded_image.FileHeader, loaded_image.MappedAddress,
> -
>                      import_desc->OriginalFirstThunk, 0 );

I don't know these functions. Is it worth checking the return value of
ImageRvaToVa? MSDN implies it can fail.
> - todo_wine ok(status_routine_called[BindImportModule] == 1,
> -
>             "StatusRoutine was called %d times\n", status_routine_called[BindImportModule]);
> - ok(status_routine_called[BindImportModule] == 1,
> -
>   "StatusRoutine was called %d times\n", status_routine_called[BindImportModule]);
>
> - todo_wine ok((status_routine_called[BindImportProcedure] == 1)
> - ok((status_routine_called[BindImportProcedure] == 1)
>
> #if defined(_WIN64)
> -
>             || broken(status_routine_called[BindImportProcedure] == 0) /* < Win8 */
> -
>   || broken(status_routine_called[BindImportProcedure] == 0) /* < Win8 */
>
> #endif
> -
>             , "StatusRoutine was called %d times\n", status_routine_called[BindImportProcedure]);
> -
>   , "StatusRoutine was called %d times\n", status_routine_called[BindImportProcedure]);

I would avoid whitespace changes on lines you're not otherwise
changing.

Andrew