Thread

1. -Wcast-qual cleanup, part 1

   Peter Eisentraut <peter_e@gmx.net> — 2011-11-07T05:13:05Z

   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.
   
   

2. Re: -Wcast-qual cleanup, part 1

   Robert Haas <robertmhaas@gmail.com> — 2011-11-07T12:47:59Z

   On Mon, Nov 7, 2011 at 12:13 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
   > Anyway, attached is the first patch for your amusement.
   
   I can't help but wonder if the cure isn't worse than the disease.  I
   mean, I very much like the fact that our code compiles without
   warnings, and I'm glad you're willing to put in the time to make that
   happen ... but aren't these warnings incredibly pedantic?
   
   const is like kudzu.  Once you start using it, you find that you need
   it everywhere ... but your life is no better than it was before,
   except that now you have const.
   
   I'm suspicious of this hunk, for example:
   
    typedef struct ErrorContextCallback
    {
    	struct ErrorContextCallback *previous;
   -	void		(*callback) (void *arg);
   -	void	   *arg;
   +	void		(*callback) (const void *arg);
   +	const void   *arg;
    } ErrorContextCallback;
   
   Why should the callback be forced to treat its private argument as const?
   
    #define XLogRecGetData(record)	((char*) (record) + SizeOfXLogRecord)
   +#define XLogRecGetConstData(record)	((const char*) (record) + SizeOfXLogRecord)
   
   IMHO, this is an example of everything that's wrong with const.  The
   result will, I suppose, be const if and only if record is const.  But
   there's no way to express that cleanly, so we have to duplicate the
   macro definition.  And anyone who is not using the right compiler
   version will have to stare at the code and scratch their head to
   figure out which one to use.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

3. Re: -Wcast-qual cleanup, part 1

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-11-07T15:07:50Z

   Robert Haas <robertmhaas@gmail.com> writes:
   > On Mon, Nov 7, 2011 at 12:13 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
   >>  typedef struct ErrorContextCallback
   >>  {
   >>  	struct ErrorContextCallback *previous;
   >> -	void		(*callback) (void *arg);
   >> -	void	   *arg;
   >> +	void		(*callback) (const void *arg);
   >> +	const void   *arg;
   >>  } ErrorContextCallback;
   
   > Why should the callback be forced to treat its private argument as const?
   
   Yes, the above seems like a seriously bad idea.  It's probably true that
   most existing callbacks could afford to declare their callback args as
   const, but it does not follow that we should legislate that in the API.
   All that that will accomplish is to move the need to cast away const
   from some places to some other places.
   
   >>  #define XLogRecGetData(record)	((char*) (record) + SizeOfXLogRecord)
   >> +#define XLogRecGetConstData(record)	((const char*) (record) + SizeOfXLogRecord)
   
   > IMHO, this is an example of everything that's wrong with const.
   
   Yes, this seems pretty horrid as well.  Not so much just the one instance
   as the implications of this sweeping requirement:
   
   >> 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.
   
   I'm not prepared to buy into that as a general coding rule.
   
   Maybe it would be better to just add -Wno-cast-qual to CFLAGS.
   
   			regards, tom lane
   
   

4. Re: -Wcast-qual cleanup, part 1

   Peter Eisentraut <peter_e@gmx.net> — 2011-11-10T19:57:43Z

   On mån, 2011-11-07 at 10:07 -0500, Tom Lane wrote:
   > >> 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.
   > 
   > I'm not prepared to buy into that as a general coding rule.
   > 
   > Maybe it would be better to just add -Wno-cast-qual to CFLAGS. 
   
   OK, I understand the concerns that have been raised here and in the
   other thread.  I'll work instead on removing "lying" const qualifiers on
   the upper layers that were the causes of attempting to push the consts
   down.