From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH 1/2] eal: add static endianness conversion
	macros
Date: Wed, 07 Jun 2017 16:16:58 +0200
Message-ID: <44029570.D8ug5AmCbY@xps>
References: <840342851720fc237214aeb30d38565615293b58.1495101988.git.adrien.mazarguil@6wind.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Return-path: <dev-bounces@dpdk.org>
Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com
 [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 081912BA1
 for <dev@dpdk.org>; Wed,  7 Jun 2017 16:16:59 +0200 (CEST)
In-Reply-To: <840342851720fc237214aeb30d38565615293b58.1495101988.git.adrien.mazarguil@6wind.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi, some comments below:

18/05/2017 12:14, Adrien Mazarguil:
> These macros resolve to constant expressions that allow developers to
> perform endianness conversion on static/const objects, even outside of
> function scope as they do not translate to function calls.
> 
> This is most useful for static initializers and constant values (whenever
> it has to be performed at compilation time). Run-time endianness conversion
> of variable values should keep using rte_*_to_*() calls for best
> performance.
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
[...]
> +#define RTE_STATIC_BSWAP64(v) \
> +	((((uint64_t)(v) & UINT64_C(0x00000000000000ff)) << 56) | \
> +	 (((uint64_t)(v) & UINT64_C(0x000000000000ff00)) << 40) | \
> +	 (((uint64_t)(v) & UINT64_C(0x0000000000ff0000)) << 24) | \
> +	 (((uint64_t)(v) & UINT64_C(0x00000000ff000000)) << 8) | \
> +	 (((uint64_t)(v) & UINT64_C(0x000000ff00000000)) >> 8) | \
> +	 (((uint64_t)(v) & UINT64_C(0x0000ff0000000000)) >> 24) | \
> +	 (((uint64_t)(v) & UINT64_C(0x00ff000000000000)) >> 40) | \
> +	 (((uint64_t)(v) & UINT64_C(0xff00000000000000)) >> 56))

Minor nit: you could align lines by inserting a space before 8.

> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +#define RTE_BE16(v) (uint16_t)(v)
> +#define RTE_BE32(v) (uint32_t)(v)
> +#define RTE_BE64(v) (uint64_t)(v)
> +#define RTE_LE16(v) RTE_STATIC_BSWAP16(v)
> +#define RTE_LE32(v) RTE_STATIC_BSWAP32(v)
> +#define RTE_LE64(v) RTE_STATIC_BSWAP64(v)
> +#elif RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> +#define RTE_BE16(v) RTE_STATIC_BSWAP16(v)
> +#define RTE_BE32(v) RTE_STATIC_BSWAP32(v)
> +#define RTE_BE64(v) RTE_STATIC_BSWAP64(v)
> +#define RTE_LE16(v) (uint16_t)(v)
> +#define RTE_LE32(v) (uint32_t)(v)
> +#define RTE_LE64(v) (uint64_t)(v)

This naming is confusing.
Let's take RTE_BE16() as example, it does not say wether the input value
is big endian or the output value will be big endian.
I think we should mimic the wording of run-time conversions:
	RTE_BE_TO_CPU_16()

Any other ideas?