[m-rev.] for review: fix warnings on x86_64

[Mark Brown]

On 31-Aug-2005, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> On 31-Aug-2005, Mark Brown <mark at cs.mu.OZ.AU> wrote:
> > Either the type of MR_PredFunc needs to be changed, or the comment in
> > mdbcomp/prim_data.m that says "the code (in browser) assumes the
> > representation is the same" should be changed, since the representation
> > is not the same.  In any case, the meaning of "EXACTLY" should probably be
> > clarified.
> 
> You want the comment to say that the representation of preds and funcs
> is the same,

I've looked into this a bit further.  There is a comment in the compiler
to the effect that such enums should have the same size as ints, so what we
really want here is for the constants defined by the enums to have the same
values as the representation of the corresponding Mercury values, even
though the enum may not have the same size as MR_Word.  Any code that wants
to interface with Mercury code will need to cast to/from MR_Word (and will
get a warning on x86_64 if it doesn't do so correctly).

I've added comments to clarify this situation for MR_PredFunc, and also for
other such enums.

> while only the runtime has a representation of "neither".

I've added a macro which defines the magic value for "neither", and put it
alongside the definition of MR_PredFunc with an explanation.

> 
> > > > +** MR_Proc_Id is a union. The first alternative identifies ordinary
> > > > +** procedures, while the second alternative identifies automatically generated
> > > > +** unification, comparison and index procedures.  The third alternative
> > > 
> > > Should initialise precedures be included in this list?
> > 
> > I don't think so.
> 
> Yes, they should be included.

Oh, right.  Ian was talking about the initialise procedures for solver
types, of course, not the recently added module based ones (which is what
I originally thought :-S ).  I've updated the comment.

Relative diff follows.

Cheers,
Mark.

--- log1	2005-08-31 23:51:03.982710572 +1000
+++ log	2005-09-01 17:00:48.542886269 +1000
@@ -35,10 +35,21 @@
 	report a warning that the test will never succeed (since -1 is not
 	one of the values in the enum).
 
-	Add an XXX pointing out that MR_PredFunc no longer matches
-	pred_or_func.
+	Clarify the comment about the requirement of MR_PredFunc to match
+	prim_data.pred_or_func.
 
-	Fix an out-of-date comment.
+	Define a macro for the value that indicates there is no proc id.
+
+	Fix a couple of out-of-date comments.
+
+trace/mercury_trace_browse.h:
+	Clarify the comments about the requirement of MR_Browse_Caller_Type,
+	MR_Browse_Format and MR_Query_Type to match their corresponding
+	Mercury types.
+
+runtime/mercury_tags.h:
+	Add a comment to point out that enums don't necessarily have the
+	same size as MR_words.
 
 runtime/mercury_stack_layout.h:
 	Use the new macro instead of testing directly whether the proc id
diff -u runtime/mercury_proc_id.h runtime/mercury_proc_id.h
--- runtime/mercury_proc_id.h	30 Aug 2005 14:06:54 -0000
+++ runtime/mercury_proc_id.h	1 Sep 2005 05:55:00 -0000
@@ -13,12 +13,11 @@
 #include "mercury_tags.h"		/* for MR_DEFINE_BUILTIN_ENUM_CONST */
 
 /*
-** This type indicates whether a procedure came from a predicate or a function.
-** This enum should EXACTLY match the definition of the `pred_or_func' type
-** in mdbcomp/prim_data.m.
-**
-** XXX enums have type int, which is 32 bits on x86_64, whereas the
-** pred_or_func type is 64 bits.
+** The MR_PredFunc type indicates whether a procedure came from a predicate
+** or a function.  The constants defined by this enum should have values
+** that correspond directly to the values in the representation of the
+** `pred_or_func' type in mdbcomp/prim_data.m, so that it is possible to
+** cast to/from MR_Word in order to interface with Mercury code.
 */
 
 typedef	enum {
@@ -27,15 +26,22 @@
 } MR_PredFunc;
 
 /*
+** This value should be distinct from any of the values in MR_PredFuncEnum.
+** It is used in MR_Proc_Id in the place where an MR_PredFunc field would be,
+** and indicates that no proc id exists.
+*/
+#define MR_NO_PROC_ID -1
+
+/*
 ** MR_Proc_Id is a union. The first alternative identifies ordinary
 ** procedures, while the second alternative identifies automatically generated
-** unification, comparison and index procedures.  The third alternative
-** indicates that no proc id exists.  The meanings of the fields in the
-** first two forms are the same as in procedure labels.  The runtime system
-** can figure out if a proc id exists by using the macro MR_PROC_ID_EXISTS,
-** and it can figure out which form is present by using the macro
-** MR_PROC_ID_IS_UCI, which will return true only if the proc id exists and
-** the procedure is of the second type.
+** unification, comparison, index and initialisation procedures.  The third
+** alternative indicates that no proc id exists.  The meanings of the fields
+** in the first two forms are the same as in procedure labels.  The runtime
+** system can figure out if a proc id exists by using the macro
+** MR_PROC_ID_EXISTS, and it can figure out which form is present by using
+** the macro MR_PROC_ID_IS_UCI, which will return true only if the proc id
+** exists and the procedure is of the second type.
 **
 ** The compiler generates MR_User_Proc_Id and MR_UCI_Proc_Id structures
 ** in order to avoid having to initialize the MR_Proc_Id union through the
@@ -68,7 +74,8 @@
 	/*
 	** This field should align with the first field of
 	** MR_User_Proc_Id_Struct, which means that it should have the same
-	** size as MR_PredFunc, an enum.
+	** size as MR_PredFunc.  Its value should always be equal to
+	** MR_NO_PROC_ID.
 	*/
 	int			MR_no_proc_id_flag;
 };
@@ -80,7 +87,7 @@
 };
 
 #define MR_PROC_ID_EXISTS(proc_id)					\
-	((proc_id).MR_proc_none.MR_no_proc_id_flag != -1)
+	((proc_id).MR_proc_none.MR_no_proc_id_flag != MR_NO_PROC_ID)
 
 #define	MR_PROC_ID_IS_UCI(proc_id)					\
 	((MR_Unsigned) (proc_id).MR_proc_user.MR_user_pred_or_func	\
only in patch2:
unchanged:
--- runtime/mercury_tags.h	14 Dec 2004 01:07:25 -0000	1.23
+++ runtime/mercury_tags.h	31 Aug 2005 13:00:54 -0000
@@ -296,7 +296,11 @@
 ** values to the enumeration constants as Mercury's tag allocation scheme
 ** assigns. (This is necessary because in .rt grades Mercury enumerations are
 ** not assigned the same values as 'normal' C enumerations).
-** 
+**
+** Note that enums have the same size as ints, but not necessarily the same
+** size as MR_Words.  Types that are defined this way should not be used by
+** Mercury code directly; instead a separate type with the correct size should
+** be defined.
 */
 
 #ifdef MR_RESERVE_TAG
only in patch2:
unchanged:
--- trace/mercury_trace_browse.h	22 Feb 2005 22:27:50 -0000	1.19
+++ trace/mercury_trace_browse.h	1 Sep 2005 06:08:04 -0000
@@ -39,8 +39,10 @@
 			MR_Word browser_term);
 
 /*
-** The following types must correspond with browse_caller_type and
-** portray_format in browser/browser_info.m.
+** The constants defined by the following enums must correspond with the
+** values in the representation of browse_caller_type and portray_format in
+** browser/browser_info.m, so that it is possible to cast to/from MR_Word
+** in order to interface with Mercury code.
 */
 typedef enum {
 	MR_DEFINE_MERCURY_ENUM_CONST(MR_BROWSE_CALLER_PRINT),
@@ -98,7 +100,12 @@
 ** Invoke an interactive query.
 */
 
-/* This must kept in sync with query_type in browser/interactive.m. */
+/*
+** The constants defined by the following enum must correspond with the
+** values in the representation of query_type in browser/interactive.m, so
+** that it is possible to cast to/from MR_Word in order to interface with
+** Mercury code.
+*/
 typedef enum { 
 	MR_DEFINE_MERCURY_ENUM_CONST(MR_NORMAL_QUERY), 
 	MR_DEFINE_MERCURY_ENUM_CONST(MR_CC_QUERY), 
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------