[m-rev.] for review: add configuration option to enable internal file copying

Julien Fischer jfischer at opturion.com
Sun Jan 14 16:04:24 AEDT 2024


On Sun, 14 Jan 2024, Zoltan Somogyi wrote:

> On 2024-01-14 15:45 +11:00 AEDT, "Julien Fischer" <jfischer at opturion.com> wrote:
>> +AC_ARG_WITH([cp],
>> +    AS_HELP_STRING([--with-cp],
>> +       [Use the cp command for file copying by the Mercury compiler.
>> +        The default is system dependent.]),
>> +       [with_cp="$withval"],
>> +       [with_cp="default"])
>
> I would prefer if the explanation here spelled out which platforms
> have which setting as default.

I have changed it to say that cp is the default except when using MSVC or
MinGW.

>> +INSTALL_METHOD=
>> +case "$with_cp" in
>> +    yes)
>> +        ;;
>> +    no)
>> +        INSTALL_METHOD="--install-method \"internal\""
>> +        ;;
>> +    default)
>> +        if test "$USING_MICROSOFT_CL_COMPILER" = "yes"
>> +        then
>> +            INSTALL_METHOD="--install-method \"internal\""
>> +        else
>> +            case "$host" in
>> +                *mingw*)
>> +                    INSTALL_METHOD="--install-method \"internal\""
>> +                    ;;
>> +            esac
>> +        fi
>> +        ;;
>> +    *)
>> +        AC_MSG_ERROR([unexpected argument to --with-cp])
>> +        ;;
>> +
>> +esac
>> +
>> +AC_SUBST(INSTALL_METHOD)
>
> I would s/INSTALL_METHOD/INSTALL_METHOD_OPT/ here,
> and below.

Done.

> I would also explicitly set external as well as internal methods
> explicitly, and assign to INSTALL_METHOD_OPT explicitly
> on each possible path through the code above, not relying
> on the initialization of that variable.

Done.

> The rest is fine.

Thanks for the review.

Julien.

>
> Zoltan.


More information about the reviews mailing list