[m-rev.] for review: avoid C warnings

Fergus Henderson fjh at cs.mu.OZ.AU
Wed Aug 14 18:03:04 AEST 2002


On 14-Aug-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: runtime/mercury.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury.h,v
> retrieving revision 1.59
> diff -u -r1.59 mercury.h
> --- runtime/mercury.h	2002/08/09 05:26:47	1.59
> +++ runtime/mercury.h	2002/08/13 09:42:28
> @@ -65,6 +65,8 @@
>  #include <setjmp.h>	/* for jmp_buf etc., which are used for commits */
>  #include <string.h>	/* for strcmp(), which is used for =/2 on strings */
>  
> +#include "mercury_misc.h"		/* for MR_fatal_error() */

It would be slightly nicer (more consistent) to put that #include together
with the other #include "mercury_*.h" directives, rather than after the
system header files.

> runtime/mercury_type_info.h:
>         Declare all the relevant fields of the RTTI data structures as static,
>         if they weren't already.

Did you mean to say "const", rather than "static"?

> +++ runtime/mercury_type_info.h	2002/08/13 09:38:55
> @@ -772,12 +772,12 @@
>  
>  typedef struct {
>      MR_ConstString          MR_du_functor_name;
> -    MR_int_least16_t        MR_du_functor_orig_arity;
> -    MR_int_least16_t        MR_du_functor_arg_type_contains_var;
> -    MR_Sectag_Locn          MR_du_functor_sectag_locn;
> -    MR_int_least8_t         MR_du_functor_primary;
> -    MR_int_least32_t        MR_du_functor_secondary;
> -    MR_int_least32_t        MR_du_functor_ordinal;
> +    const MR_int_least16_t  MR_du_functor_orig_arity;
> +    const MR_int_least16_t  MR_du_functor_arg_type_contains_var;
> +    const MR_Sectag_Locn    MR_du_functor_sectag_locn;
> +    const MR_int_least8_t   MR_du_functor_primary;
> +    const MR_int_least32_t  MR_du_functor_secondary;
> +    const MR_int_least32_t  MR_du_functor_ordinal;

It's not necessary and probably not desirable to declare these fields as
`const'.  Also, it is not consistent to declare these fields as `const'
without declaring the remaining fields as const.
The remaining fields are pointers, and what they point to is declared as
const, but the fields themselves are not declared const:

>      const MR_PseudoTypeInfo *MR_du_functor_arg_types;
>      const MR_ConstString    *MR_du_functor_arg_names;
>      const MR_DuExistInfo    *MR_du_functor_exist_info;

If you really want to declare all the fields as const, you'd need to use

      const MR_PseudoTypeInfo *const MR_du_functor_arg_types;
      const MR_ConstString    *const MR_du_functor_arg_names;
      const MR_DuExistInfo    *const MR_du_functor_exist_info;

and
      const MR_ConstString    MR_du_functor_name;

However, I'm not at all convinced that this is really worthwhile.
I don't see how declaring these fields as const could avoid compiler warnings.
Which warnings does it avoid?

In general, declaring fields as `const' is probably not a good idea.
It makes it very difficult to dynamically allocate values of that type
(since there would be no type-safe way to initialize the const fields).

Now, although type_ctor_infos are currently all allocated statically,
I don't think that this will necessarily remain the case in the future.
For example, if we were to implement a byte-code interpreter for Mercury,
then we might want some way of reading type_ctor_infos in from files.
It would be kind of pointless to add in all these consts now and then
just have to take them out again later.

`const' is useful and important in two cases:

	- On static or global objects, so that they go in .rdata
	  rather than .data; this improves efficiency and helps
	  catch accidental writes to such data.

	- With pointer types (the const should go on the type
	  pointed to, not on the pointer type), so that it is
	  clear which pointers are used only for read access.
	  This information is useful for people reading the code.

But with fields, the constness of the field is normally inherited from the
constness of its containing object.   So you don't normally need to declare
fields as const.

Declaring a field to be const means that the field never changes, even
if the containing object is not const.  Not just "never changes after it
is constructed", like it means in C++, but "never changes after it is
statically initialized".  That's pretty restrictive, and there doesn't
seem to be much advantage for it, either in terms of optimization
or in terms of readability.  So I don't see the point.

> @@ -816,8 +816,8 @@
>  typedef struct {
> -    MR_ConstString      MR_enum_functor_name;
> -    MR_int_least32_t    MR_enum_functor_ordinal;
> +    MR_ConstString          MR_enum_functor_name;
> +    const MR_int_least32_t  MR_enum_functor_ordinal;

Likewise here.  Again it is inconsistent to declare one of these
fields as const but not the other.

>  typedef struct {
> -    MR_ConstString      MR_notag_functor_name;
> -    MR_PseudoTypeInfo   MR_notag_functor_arg_type;
> -    MR_ConstString      MR_notag_functor_arg_name;
> +    MR_ConstString          MR_notag_functor_name;
> +    const MR_PseudoTypeInfo MR_notag_functor_arg_type;
> +    MR_ConstString          MR_notag_functor_arg_name;
>  } MR_NotagFunctorDesc;

Likewise.

>  typedef struct {
> -    MR_ConstString      MR_ra_functor_name;
> -    MR_int_least32_t    MR_ra_functor_ordinal;
> -    MR_ReservedAddr     MR_ra_functor_reserved_addr;
> +    MR_ConstString          MR_ra_functor_name;
> +    const MR_int_least32_t  MR_ra_functor_ordinal;
> +    MR_ReservedAddr         MR_ra_functor_reserved_addr;
>  } MR_ReservedAddrFunctorDesc;

Likewise.

>  typedef struct {
> -    MR_int_least32_t                MR_sectag_sharers;
> -    MR_Sectag_Locn                  MR_sectag_locn;
> +    const MR_int_least32_t          MR_sectag_sharers;
> +    const MR_Sectag_Locn            MR_sectag_locn;
>      const MR_DuFunctorDesc * const *MR_sectag_alternatives;
>  } MR_DuPtagLayout;

Likewise.

> @@ -925,11 +925,11 @@
>  typedef struct {
> -    MR_int_least16_t                    MR_ra_num_res_numeric_addrs;
> -    MR_int_least16_t                    MR_ra_num_res_symbolic_addrs;
> +    const MR_int_least16_t              MR_ra_num_res_numeric_addrs;
> +    const MR_int_least16_t              MR_ra_num_res_symbolic_addrs;
>      const void * const                  *MR_ra_res_symbolic_addrs;
>      MR_ReservedAddrFunctorDescPtr const *MR_ra_constants;
> -    MR_DuTypeLayout                     MR_ra_other_functors;  
> +    const MR_DuTypeLayout               MR_ra_other_functors;  
>  } MR_ReservedAddrTypeDesc;

Likewise.

>  typedef union {
> -    void                        *MR_layout_init;
> -    MR_DuTypeLayout             MR_layout_du;
> -    MR_EnumTypeLayout           MR_layout_enum;
> -    MR_NotagTypeLayout          MR_layout_notag;
> -    MR_ReservedAddrTypeLayout   MR_layout_reserved_addr;
> -    MR_EquivLayout              MR_layout_equiv;
> +    const void                          *MR_layout_init;
> +    const MR_DuTypeLayout               MR_layout_du;
> +    const MR_EnumTypeLayout             MR_layout_enum;
> +    const MR_NotagTypeLayout            MR_layout_notag;
> +    const MR_ReservedAddrTypeLayout     MR_layout_reserved_addr;
> +    const MR_EquivLayout                MR_layout_equiv;
>  } MR_TypeLayout;

Likewise.  The change from `void *' to `const void *' is ok,
but the other changes are inconsistent and unnecessary.

>  typedef struct {
> -    MR_ConstString              MR_maybe_res_name;
> -    MR_Integer                  MR_maybe_res_arity;
> -    MR_bool                     MR_maybe_res_is_res;
> -    MR_MaybeResFunctorDescPtr   MR_maybe_res_ptr;
> +    MR_ConstString                      MR_maybe_res_name;
> +    const MR_Integer                    MR_maybe_res_arity;
> +    const MR_bool                       MR_maybe_res_is_res;
> +    const MR_MaybeResFunctorDescPtr     MR_maybe_res_ptr;
>  } MR_MaybeResAddrFunctorDesc;

Likewise.

>  #define MR_maybe_res_du         MR_maybe_res_ptr.MR_maybe_res_du_ptr
> @@ -1025,11 +1025,11 @@
>  */
>  
>  typedef union {
> -    void                        *MR_functors_init;
> -    MR_DuFunctorDesc            **MR_functors_du;
> -    MR_MaybeResAddrFunctorDesc  *MR_functors_res;
> -    MR_EnumFunctorDesc          **MR_functors_enum;
> -    MR_NotagFunctorDesc         *MR_functors_notag;
> +    const void                        *MR_functors_init;
> +    const MR_DuFunctorDesc            **MR_functors_du;
> +    const MR_MaybeResAddrFunctorDesc  *MR_functors_res;
> +    const MR_EnumFunctorDesc          **MR_functors_enum;
> +    const MR_NotagFunctorDesc         *MR_functors_notag;
>  } MR_TypeFunctors;

Likewise.

>  struct MR_TypeCtorInfo_Struct {
> -    MR_Integer          MR_type_ctor_arity;
> -    MR_int_least8_t     MR_type_ctor_version;
> -    MR_int_least8_t     MR_type_ctor_num_ptags;         /* if DU */
> -    MR_TypeCtorRepInt   MR_type_ctor_rep_CAST_ME;
> -    MR_ProcAddr         MR_type_ctor_unify_pred;
> -    MR_ProcAddr         MR_type_ctor_compare_pred;
> -    MR_ConstString      MR_type_ctor_module_name;
> -    MR_ConstString      MR_type_ctor_name;
> -    MR_TypeFunctors     MR_type_ctor_functors;
> -    MR_TypeLayout       MR_type_ctor_layout;
> -    MR_int_least32_t    MR_type_ctor_num_functors;
> +    const MR_Integer                  MR_type_ctor_arity;
> +    const MR_int_least8_t             MR_type_ctor_version;
> +    const MR_int_least8_t             MR_type_ctor_num_ptags; /* if DU */
> +    const MR_TypeCtorRepInt           MR_type_ctor_rep_CAST_ME;
> +    MR_STATIC_CODE_CONST MR_ProcAddr  MR_type_ctor_unify_pred;
> +    MR_STATIC_CODE_CONST MR_ProcAddr  MR_type_ctor_compare_pred;

> +    const MR_ConstString              MR_type_ctor_module_name;
> +    const MR_ConstString              MR_type_ctor_name;
> +    const MR_TypeFunctors             MR_type_ctor_functors;
> +    const MR_TypeLayout               MR_type_ctor_layout;
> +    const MR_int_least32_t            MR_type_ctor_num_functors;
>  
>  /*
>  ** The following fields will be added later, once we can exploit them:
> -**  MR_TrieNodePtr      MR_type_ctor_std_table;
> -**  MR_ProcAddr         MR_type_ctor_prettyprinter;
> +**  MR_TrieNodePtr                    MR_type_ctor_std_table;
> +**  MR_STATIC_CODE_CONST MR_ProcAddr  MR_type_ctor_prettyprinter;
>  */
>  };

Likewise (though here you were a bit more consistent;
the commented out field MR_type_ctor_std_table is the only
non-const one here).

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------



More information about the reviews mailing list