[m-rev.] for review: Enable some clang warnings.

[Peter Wang]

On Mon, 10 Sep 2018 09:12:59 +1000 (AEST), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 
> On Sun, 9 Sep 2018 14:28:17 +1000, Peter Wang <novalazy at gmail.com> wrote:
> > (1) hard_coded/deep_copy.m and hard_coded/deep_copy_exist.m fail because
> > the compiler generates:
> > 
> >   static const MR_DuFunctorDesc deep_copy__deep_copy__du_functor_desc_bit_vector_test_1_0 = {
> >     (MR_String) "tuple_a",
> >     (MR_Integer) 18,
> >     (MR_Integer) 32768,
> >     ...
> > 
> > where the third field of MR_DuFunctorDesc has type MR_int_least16_t
> > so clang warns that:
> > 
> >   deep_copy.c:867:3: error: implicit conversion from 'MR_Integer'
> >   (aka 'long') to 'MR_int_least16_t' (aka 'short') changes value from
> >   32768 to -32768 [-Werror,-Wconstant-conversion]
> > 
> > Many of the fields in the type info structures should be changed to
> > unsigned types at some stage, I suppose.
> 
> Yes. Many fields in type_ctor_infos, their substructures and related
> structures are signed only because Mercury supported only signed integers
> when they were designed. Many, like this one, should be unsigned.
> Would your change to clang options allow a diff to simply change
> the signedness of the fields in the C type definitions, updating
> the affected handwritten C code accordingly, updating mmc
> to emit values of the right signedness, while still being able
> to build stage 1 despite the signedness mismatches caused
> by an un-updated installed mmc? Would gcc allow it?

Yes, that should pose no problem. Implicit conversion from a signed
value to fit an unsigned field is allowed. The warning above is
triggered if the implicit conversion causes the value to change
(overflows), which should happen LESS if a field's type is
changed to match its intended range.

> > (2) Some tests in the `valid' directory use looping predicates, which
> > clang picks up on. It is possible to suppress the warnings with
> > -Wno-infinite-recursion but it would be better to modify those test
> > cases. Should we replace the looping predicates with external_pred,
> > for example? These are the tests:
> > 
> >     valid/higher_order5
> >     valid/loop_in_disj
> >     valid/mode_syntax
> >     valid/recursive_no_tag_type
> >     valid/same_length_2
> >     valid/stack_alloc
> 
> Where the actual behavior being tested has nothing to do with looping,
> then eliminating the looping would seem to be the right thing to do,
> though I would try giving a base case to the loop(s) in the Mercury code
> before resorting to external_pred. Unfortunately, the git log did
> not tell me what the loop_in_disj, same_length_2 and stack_alloc
> test cases are actually testing FOR. I suspect we may have to dig
> in the CVS logs as well.

The git repository should have everything that was in the CVS
repositories.

> 
> >          # XXX we need go through the warning and optimization options for clang
> >          # more carefully.
> > -        CFLAGS_FOR_WARNINGS="-w"
> > +        CFLAGS_FOR_WARNINGS="-Wall -Wshadow -Wstrict-prototypes -Wmissing-prototypes -Wno-parentheses-equality -Wno-non-literal-null-conversion -Wno-unused-function -Wno-unused-variable -Wno-uninitialized"
> 
> Why no line wrap?

The value must not contain newlines when substituted into
Mercury.config. We could build up the value over multiple statements.

> 
> And which version of clang are these intended for? I tried to look up the new options
> in "man clang", but got nothing. My laptop has clang 3.2.7.

I have clang 6.0.1. The only reference I can find is
https://clang.llvm.org/docs/DiagnosticsReference.html but the options
are unversioned there.

https://github.com/Barro/compiler-warnings suggests that all the options
above are supported back to clang 3.2.x, except perhaps
-Wmissing-prototypes (which seems strange as it is pretty basic).

Peter