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

[Zoltan Somogyi]

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?

BTW, there are some fields that *look* like they should contain
only unsigned values, which are nevertheless signed for a valid reason,
which is that the value that they are designed to hold is actually of
a type that would best be expressed in Mercury as maybe(uint).
These we currently translate to C ints, with negative values
representing "no".

There are two other issues here as well. One is size. The third
field of MR_DuFunctorDesc is now MR_int_least16_t, and should
be MR_uint_least16_t. But in the compiler, its representation
is wrong not only in signedness, but also in size. Again,
the reason is the same: sub-word-sized integers did not exist
when that code was written. Fixing this should not have any
problems with bootstrapping.

The other is an issue internal to the compiler. The definition
of the type mlds_type is wrong in my opinion, for severakl reasons.
One, it has mlds_native_int_type and mlds_native_uint_type,
but not their 8, 16, 32 and 64 bit versions. Second, it requires
redundancy that should not be necessary, by storing both
a full mercury type, and its synopsis (the type category)
in the mercury_type function symbol. Third, it has a field
for foreign type assertions even for non-foreign types.
Fourth, it allows some types, such as uint, to be represented
in two different ways: as mlds_native_type_uint, and as
a Mercury type.

Updating the compiler internals to represent RTTI values
using integers of the correct size and signedness would
be significantly simpler once mlds_type's definition
has been changed to fix these issues.
 
> (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.

>          # 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?

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.

Zoltan.