-Wcast-qual cleanup, part 1 - Mailing list pgsql-hackers

From Peter Eisentraut
Subject -Wcast-qual cleanup, part 1
Date
Msg-id 1320642785.25734.13.camel@vanquo.pezone.net
Whole thread Raw
Responses Re: -Wcast-qual cleanup, part 1
List pgsql-hackers
I've been looking into getting rid of the warnings pointed out by
-Wcast-qual, which are mainly caused by casting away "const" qualifiers.
(It also warns about casting away "volatile" qualifiers, and we have a
number of those as well, but that's for another day.)  I've gotten them
down to a manageable amount, and with a bit of effort we could probably
get them down to a few dozen.

I have split my change set into about a dozen separate patches, which
I'll post for discussion in separate batches.  But I'll start with the
biggest one, which includes the following notable interwoven changes:

The error callbackup functions signature changes thus:

 static void pg_start_backup_callback(int code, Datum arg);
 static bool read_backup_label(XLogRecPtr *checkPointLoc,
                                  bool *backupEndRequired);
-static void rm_redo_error_callback(void *arg);
+static void rm_redo_error_callback(const void *arg);
 static int     get_sync_bit(int method);

This is because various places provide const values as the callback
argument.

The xlog _desc() functions signature changes thus:

 }

 void
-gin_desc(StringInfo buf, uint8 xl_info, char *rec)
+gin_desc(StringInfo buf, uint8 xl_info, const char *rec)
 {
        uint8           info = xl_info & ~XLR_INFO_MASK;

This is because rm_redo_error_callback() makes calls to the _desc()
functions.

A couple of themes that also appear in the other patches are worth
thinking about:

1. Hiding away the pointerness of a type behind a typedef like this

typedef struct CopyStateData *CopyState;

is no good, because that loses the ability to apply const (or other)
qualifiers to the pointer.  See this hunk:

  * The argument for the error context must be CopyState.
  */
 void
-CopyFromErrorCallback(void *arg)
+CopyFromErrorCallback(const void *arg)
 {
-       CopyState       cstate = (CopyState) arg;
+       const CopyStateData *cstate = (const CopyStateData *) arg;

        if (cstate->binary)
        {

I think we recently had another discussion (Relation/RelationData?) that
also concluded that this coding style is flawed.

2. Macros accessing  structures should come in two variants: a "get"
version, and a "set"/anything else version, so that the "get" version
can preserve the const qualifier.  See this hunk:

 #define SizeOfXLogRecord       MAXALIGN(sizeof(XLogRecord))

 #define XLogRecGetData(record) ((char*) (record) + SizeOfXLogRecord)
+#define XLogRecGetConstData(record)    ((const char*) (record) +
SizeOfXLogRecord)

 /*
  * XLOG uses only low 4 bits of xl_info.  High 4 bits may be used by rmgr.

(This is not a particularly good naming convention.  I would probably go
with XLogRecData() and XLogRecGetData() if we were designing this from
scratch.)

Anyway, attached is the first patch for your amusement.


Attachment

pgsql-hackers by date:

Previous
From: Jan Kundrát
Date:
Subject: [patch] Include detailed information about a row failing a CHECK constraint into the error message
Next
From: J Smith
Date:
Subject: Re: unaccent extension missing some accents