Re: [PATCH 1/2] cmd: Use terminated strings in WCMD_ReadAndParseLine()

[Hugh McMaster]

On Mon, 12 Apr 2021 02:22:36 +0200, Gijs Vermeulen wrote:
> @@ -1870,10 +1865,7 @@ WCHAR *WCMD_ReadAndParseLine(const WCHAR *optionalcmd, CMD_LIST **output, HANDLE
>  /* Show prompt before batch line IF echo is on and in batch program */
>  if (context && echo_mode && *curPos && (*curPos != '@')) {
> -
>  static const WCHAR echoDot[] = {'e','c','h','o','.'};
> -
>  static const WCHAR echoCol[] = {'e','c','h','o',':'};
> -
>  static const WCHAR echoSlash[] = {'e','c','h','o','/'};
> -
>  const DWORD len = ARRAY_SIZE(echoDot);
> -
>  const DWORD len = ARRAY_SIZE(L"echo.") - 1;

This is very unusual. Why not just use lstrlenW(...)?

You should also look at calculating the string length within
WCMD_keyword_ws_found(), instead of passing the length as an argument.
But do this as a separate patch.

--
Hugh McMaster

[PATCH 1/2] cmd: Use terminated strings in WCMD_ReadAndParseLine()

[Gijs Vermeulen]

Op ma 12 apr. 2021 om 08:17 schreef Hugh McMaster <[e-mail]
> :
> This is very unusual. Why not just use lstrlenW(...)?

I just followed these examples[1][2], but I can change it if necessary.

You should also look at calculating the string length within
> WCMD_keyword_ws_found(), instead of passing the length as an argument.
> But do this as a separate patch.

Will do.

[1]
https://source.winehq.org/git/wine.git/commit/76ee089aade14ddf833c4ef7c382c8...
[2]
https://source.winehq.org/git/wine.git/commit/762f51cf93b6efca0defb2929f3278...

[PATCH 1/2] cmd: Use terminated strings in WCMD_ReadAndParseLine()

[Hugh McMaster]

On Mon, 12 Apr 2021 at 18:04, Gijs Vermeulen wrote:
> Op ma 12 apr. 2021 om 08:17 schreef Hugh McMaster:
> > This is very unusual. Why not just use lstrlenW(...)?
>
> I just followed these examples[1][2], but I can change it if necessary.

Thanks. It's just adding unnecessary complexity in this case.
> > You should also look at calculating the string length within
> > WCMD_keyword_ws_found(), instead of passing the length as an argument.
> > But do this as a separate patch.
>
> Will do.
>
> [1] https://source.winehq.org/git/wine.git/commit/76ee089aade14ddf833c4ef7c382c8...
> [2] https://source.winehq.org/git/wine.git/commit/762f51cf93b6efca0defb2929f3278...

[PATCH 1/2] cmd: Use terminated strings in WCMD_ReadAndParseLine()

[Gijs Vermeulen]

Op di 13 apr. 2021 om 14:33 schreef Hugh McMaster <[e-mail]
> :
> Thanks. It's just adding unnecessary complexity in this case.

Not sure if it matters much, but isn't using lstrlenW() going to be more
expensive than using ARRAY_SIZE() for the same thing?

[PATCH 1/2] cmd: Use terminated strings in WCMD_ReadAndParseLine()

[Hugh McMaster]

On Wed, 14 Apr 2021 at 00:17, Gijs Vermeulen wrote:
> Not sure if it matters much, but isn't using lstrlenW() going to be more expensive than using ARRAY_SIZE() for the same thing?

While the sizeof() calculations in the ARRAY_SIZE() macro occur at
compile-time (meaning no cost), lstrlenW() and friends are highly
optimized for speed. Some implementations of strlen() are even written
in assembly for even more speed.

Given the string lengths being checked seem to be <= 5, any cost is
extremely minimal at best.

I'm not sure if anyone else has any thoughts on this, but I still
think calculating string length in WCMD_keyword_ws_found() will work
better.

[PATCH 1/2] cmd: Use terminated strings in WCMD_ReadAndParseLine()

[Gijs Vermeulen]

Op wo 14 apr. 2021 om 07:21 schreef Hugh McMaster <[e-mail]
> :
> While the sizeof() calculations in the ARRAY_SIZE() macro occur at
> compile-time (meaning no cost), lstrlenW() and friends are highly
> optimized for speed. Some implementations of strlen() are even written
> in assembly for even more speed.
>
> Given the string lengths being checked seem to be <= 5, any cost is
> extremely minimal at best.

Understood.
> I'm not sure if anyone else has any thoughts on this, but I still
> think calculating string length in WCMD_keyword_ws_found() will work
> better.

I will just rework the series to do this instead. Thanks for all the
comments/reviews.

[PATCH 1/2] cmd: Use terminated strings in WCMD_ReadAndParseLine()

[Henri Verbeet]

On Wed, 14 Apr 2021 at 07:22, Hugh McMaster  wrote:
> On Wed, 14 Apr 2021 at 00:17, Gijs Vermeulen wrote:
> > Not sure if it matters much, but isn't using lstrlenW() going to be more expensive than using ARRAY_SIZE() for the same thing?
>
> While the sizeof() calculations in the ARRAY_SIZE() macro occur at
> compile-time (meaning no cost), lstrlenW() and friends are highly
> optimized for speed. Some implementations of strlen() are even written
> in assembly for even more speed.

That's true for glibc's strlen(), and in cases where gcc can determine
the string length at compile time it may replace instances of strlen()
with a constant, but it's not quite true for Wine's strlenW(), which
has a fairly straightforward/naïve implementation. Still, it seems
unlikely to be especially consequential, one way or the other.