[PATCH 2/2] msgb: Optionally declare some msgb struct fields as const

[Holger Hans Peter Freyther]

On Fri, Feb 28, 2014 at 08:47:46PM +0100, Jacob Erlbeck wrote:

Good Morning,

I welcome any change that makes it more difficult to create the
mess that LAPD/LAPDm is in. I have some questions and comments
though.

> Writing directly to following struct fields may cause inconsistencies
> that hard to debug:

> In general, the available macros and functions should be used to
> modify them instead.

						^ must as in RFC?

> This patch declares these fields as const if
> MSGB_DISABLE_DIRECT_WRITE is defined. Doing so may lead to warnings
> and errors, therefore this macro is only defined for libosmocore yet,
> where at least the errors are also fixed by this patch.

I will get back to this later as well. Have you looked at GSEAL
of GTK+ 2.0? Their goal was to hide direct access to variables too
and they introduced a macro like:

#ifndef GSEAL
/* introduce GSEAL() here for all of Gdk and Gtk+ without the need to modify GLib */
#  ifdef GSEAL_ENABLE
#    define GSEAL(ident)      _g_sealed__ ## ident
#  else
#    define GSEAL(ident)      ident
#  endif
#endif /* !GSEAL */

It is not making the variables const but hiding them with a rename
and causing a compilation failure when being accessed.

> -AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include
> +export AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include -DMSGB_DISABLE_DIRECT_WRITE

Is that really necessary?

> +#ifdef DOXYGEN

My take would be that we try to avoid direct access so we don't need
to document these variables and can use \internal here. I think it is
preferable to avoid the situation where the ABI changes based on an
external define (even if it is just for documentation now). I have seen
a lot of wild CFLAGS/CPPFLAGS that users set while working on OpenEmbedded


> +	const uint16_t data_len;   /*!< \brief length of underlying data array */
> +	const uint16_t len;        /*!< \brief length of bytes used in msgb */
> +	unsigned char const *head; /*!< \brief start of underlying memory buffer */
> +	unsigned char const *tail; /*!< \brief end of message in buffer */
> +	unsigned char const *data; /*!< \brief start of message in buffer */

> +#else
> +	union {
> +		MSGB_CONST uint16_t data_len;
> +		uint16_t __data_len;
> +	};

Anyone knows an ABI where alignment for const/non-const is different? :)


cheers
	holger