[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