Re: [PATCH 1/3] [cmd] Convert the set /a tests to validate better

[Frédéric Delanoy]

On Thu, Dec 13, 2012 at 10:59 PM, Ann and Jason Edmeades
 wrote:
> The tests previously set a variable which was not checked until
> the subsequent test completed (as env vars are expanded as the
> line is read, meaning a sequence of set /a var=x & echo %var%
> echos the contents before the set is executed). In addition when
> multiple variables are involved in the calculation, only the
> first one was being checked, and this is changed to check all
> variables involved in the calculation.

+
+REM Check the variables are set to the expected value and clear their contents
+:check_vars
+if not "%1"=="" (
+  if "%1"=="%WINE_var1%" (
+    echo WINE_var1 correctly %1
+  ) else (
+    echo ERROR: WINE_var1 incorrectly %WINE_var1% [%1]
+  )
+)
+if not "%2"=="" (
+  if "%2"=="%WINE_var2%" (
+    echo WINE_var2 correctly %2
+  ) else (
+    echo ERROR: WINE_var2 incorrectly %WINE_var2% [%2]
+  )
+)
+if not "%3"=="" (
+  if "%3"=="%WINE_var3%" (
+    echo WINE_var3 correctly %1
+  ) else (
+    echo ERROR: WINE_var3 incorrectly %WINE_var3% [%3]
+  )
+)

A couple comments:

1. I don't really like hardcoding a variable name in a function... Why
not using something like:
set /a var=1 +2 & call :compute var
set /a foo=8, bar=foo+1 & call :compute foo bar

goto :end
:compute
set %1
set %1=     // to clear the contents
if not "%2" == "" (
  set %2
  set %2= // clearing here as well
)

goto :eof
:end

which prints the variable(s) name and value, without hardcoding?
var=3
foo=8
bar=9

2. The check for the presence of the first parameter is unneeded since
you always call with at least one param (you control all calls)
3. When only 1 variable is used, no need to add a '1' suffix as in
'foo1' or 'WINE_foo1': the '1' doesn't help/add useful information
4. Ideally, for clarity, one would use delayed expansion instead, such as
   setlocal EnableDelayedExpansion
   set /a var=1 +2 & echo !var!
   endlocal
but this isn't implemented yet, so adding a preliminary comment like
"REM FIXME Simplify tests when EnableDelayedExpansion is supported"
would be good IMO

5. You could even simplify the above function (making it more generic,
instead of testing for up to 3 arguments) by using SHIFT and an
additional label, like
:compute
:comploop
if not "%1" == "" (
  set %1
  set %1=
  shift
) else (
  goto :eof
)
goto :comploop
goto :eof

Or even better (if NT4 allows):

:compute
 for %%i in (%*) do (
   set %%i
   set %%i=
 )
 goto :eof

Frédéric