[m-rev.] for review: warn about unsigned comparisons against zero that are tautologies

Zoltan Somogyi zoltan.somogyi at runbox.com
Sat Oct 20 19:31:43 AEDT 2018



On Sat, 20 Oct 2018 05:53:00 +0000 (UTC), Julien Fischer <jfischer at opturion.com> wrote:
> +maybe_generate_warning_for_useless_comparison(
> +        PredInfo, InstMap, Args, GoalInfo, !Info) :-
> +    pred_info_is_pred_or_func(PredInfo) = PredOrFunc,
> +    ModuleSymName = pred_info_module(PredInfo),
> +    ( if
> +        simplify_do_warn_simple_code(!.Info),
> +        is_std_lib_module_name(ModuleSymName, ModuleName),
> +        PredOrFunc = pf_predicate,
> +        Args = [ArgA, ArgB]
> +    then

I have two minor issues with this diff. The first is how you test whether
warn_simple_code is enabled. *Either* it should be tested in the
condition of this if-then-else, *or* it should be tested in the conditional
error message components and severity, but not both, since one is enough.
And if it is tested in this condition, it should be the *last* test: if you are
gonna fail, you wanna fail fast.

> +is_useless_unsigned_comparison(ModuleName, PredName, ArgA, ArgB,
> +        Pieces) :-
> +    (
> +        PredName = ">=",
> +        arg_is_unsigned_zero(ModuleName, ArgB, ZeroStr),
> +        Pieces = [words("cannot fail."), nl,
> +            words("Comparison of"), words(ModuleName),
> +            words(">="), words(ZeroStr), words("is always true.")]

The other issue is the wording of these messages.
I think something along the lines of "All uint8 values are >= 0u8"
would be clearer.

Other than that, the diff is fine.

Note that I think Peter is also working on this module, so one of you
will probably get a conflict :-(

Zoltan.


More information about the reviews mailing list