Converting source to unicode, bug 28189

[Hugh McMaster]

I've put together a draft patch to address bug 28189, regsvr32: No usage in wineconsole.

I'm using resource strings and have converted all of the printf calls to WriteConsoleW.

I also converted the source to Unicode, but overall, there are few coding points I'm not quite sure about.

If someone could look over the changes and offer some feedback, I'd certainly appreciate it.

URL: http://bugs.winehq.org/show_bug.cgi?id=28189

--
Hugh

[Akihiro Sagawa]

On Thu, 13 Feb 2014 07:35:21 -0500, Hugh McMaster wrote:
> I've put together a draft patch to address bug 28189, regsvr32:
> No usage in wineconsole.
>
> I'm using resource strings and have converted all of the printf calls
> to WriteConsoleW.
>
> I also converted the source to Unicode, but overall, there are few
> coding points I'm not quite sure about.
>
> If someone could look over the changes and offer some feedback,
> I'd certainly appreciate it.
>
> URL: http://bugs.winehq.org/show_bug.cgi?id=28189

Hello Hugh,

Regarding your patch, attachment #47519, I roughly read the code and
post comments at the following points:
1. Dead assignment: In output_write function, len and ret initial
   value will overwrite by function return value. You don't need
   to initialize them.
2. Memory leaking: if (!strA) is true, calling LocalFree(str) is
   necessary before return.
3. Usage indent: I'm not sure about STRING_USAGE and spaceSpaceW.
   Another command line tool, e.g. cmd.exe, doesn't do like this.
   Additionally, it might confuse po translators.
4. /I option handing: lstrcmpiW isn't compatible with strncasecmp.
5. Double quote handling: command_line is advanced when the code
   finds a double quote character. When it updates wsCommandLine?
6. Needless changes: `/**' -> `/*', pointer asterisk position, a
   space character after C-language keyword. Please follow surround
   coding style.
7. Silent handling: Many times, it refers Silent variable before
   calling output_write function. How about moving the check to the
   function?

Hope these comments are helpful.

Regards,
Akihiro Sagawa

[Hugh McMaster]

Hi Akihiro,

Many thanks for your comments.

On Friday, 14 February 2014 3:06 AM, Akihiro Sagawa wrote:
> Regarding your patch, attachment #47519, I roughly read the code and post comments at the following points:
> 5. Double quote handling: command_line is advanced when the code
> finds a double quote character. When it updates wsCommandLine?

Good catch!  :)
> - Silent handling: Many times, it refers Silent variable before
>
> calling output_write function. How about moving the check to the
>   function?

Do you mean that I should add the Silent boolean value as a variable in the function call?

[Akihiro Sagawa]

On Fri, 14 Feb 2014 06:25:38 -0500, Hugh McMaster wrote:
> > - Silent handling: Many times, it refers Silent variable before
> >
> > calling output_write function. How about moving the check to the
> >   function?
>
> Do you mean that I should add the Silent boolean value as a variable 
> in the function call?

No. I meant:

[Existing code]
static int some_function(void)
{
  /* ... */
  if (Silent) {
    output_write(STRING_FOO);
    output_write(STRING_BAR);
  }
  /* ... */
}

[My idea]
static int some_function(void)
{
  /* ... */
  output_write(STRING_FOO);
  output_write(STRING_BAR);
  /* ... */
}

static void __cdecl output_write(UINT id, ...)
{
  /* variable declarations */

if (Silent)
    return; /* do nothing */

/* then, LoadString, FormatMessage, etc. */
}

Does this work for you?

[Hugh McMaster]

On Saturday, 15 February 2014 3:00 AM, Akihiro Sagawa wrote:
> [My idea]
> static int some_function(void)
> {
> /* ... */
> output_write(STRING_FOO);
>  output_write(STRING_BAR);
>  /* ... */
> }
>
> static void __cdecl output_write(UINT id, ...) {
>  /* variable declarations */
>
> if (Silent)
>    return; /* do nothing */
>
> /* then, LoadString, FormatMessage, etc. */ }
>
> Does this work for you?

Yes. This makes perfect sense now.

Thank you.