Return-Path: <cyrus@holtmann.org>
MIME-Version: 1.0
In-Reply-To: <1387283874-29721-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
References: <1387283874-29721-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Date: Tue, 17 Dec 2013 15:34:36 +0200
Message-ID: <CABBYNZLCgpqR+hsR7vcadV52Y+x2jdRSWGQWfsSr3PhyPKCNTQ@mail.gmail.com>
Subject: Re: [PATCH 1/7] android/tester: Fix enum and define coding style
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: Grzegorz Kolodziejczyk <grzegorz.kolodziejczyk@tieto.com>
Cc: "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset=ISO-8859-1
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

Hi Grzegorz,

On Tue, Dec 17, 2013 at 2:37 PM, Grzegorz Kolodziejczyk
<grzegorz.kolodziejczyk@tieto.com> wrote:
> This changes all enums values and defines to uppercase instead of
> lowercase according to coding style.
> ---
>  android/android-tester.c | 86 ++++++++++++++++++++++++------------------------
>  1 file changed, 43 insertions(+), 43 deletions(-)
>
> diff --git a/android/android-tester.c b/android/android-tester.c
> index eb938d0..4eb265b 100644
> --- a/android/android-tester.c
> +++ b/android/android-tester.c
> @@ -37,10 +37,10 @@
>  #include <hardware/bluetooth.h>
>  #include <hardware/bt_sock.h>
>
> -#define adapter_props adapter_prop_bdaddr, adapter_prop_bdname, \
> -                       adapter_prop_uuids, adapter_prop_cod, \
> -                       adapter_prop_type, adapter_prop_scan_mode, \
> -                       adapter_prop_bonded_devices, adapter_prop_disc_timeout
> +#define ADAPTER_PROPS ADAPTER_PROP_BDADDR, ADAPTER_PROP_BDNAME, \
> +                       ADAPTER_PROP_UUIDS, ADAPTER_PROP_COD, \
> +                       ADAPTER_PROP_TYPE, ADAPTER_PROP_SCAN_MODE, \
> +                       ADAPTER_PROP_BONDED_DEVICES, ADAPTER_PROP_DISC_TIMEOUT
>
>  static bt_scan_mode_t test_setprop_scanmode_val =
>                                         BT_SCAN_MODE_CONNECTABLE_DISCOVERABLE;
> @@ -52,19 +52,19 @@ static uint32_t test_setprop_disctimeout_val = 120;
>   */
>
>  enum hal_bluetooth_callbacks_id {
> -       adapter_test_end,
> -       adapter_test_setup_mode,
> -       adapter_state_changed_on,
> -       adapter_state_changed_off,
> -       adapter_prop_bdaddr,
> -       adapter_prop_bdname,
> -       adapter_prop_uuids,
> -       adapter_prop_cod,
> -       adapter_prop_type,
> -       adapter_prop_scan_mode,
> -       adapter_prop_disc_timeout,
> -       adapter_prop_service_record,
> -       adapter_prop_bonded_devices
> +       ADAPTER_TEST_END,
> +       ADAPTER_TEST_SETUP_MODE,
> +       ADAPTER_STATE_CHANGED_ON,
> +       ADAPTER_STATE_CHANGED_OFF,
> +       ADAPTER_PROP_BDADDR,
> +       ADAPTER_PROP_BDNAME,
> +       ADAPTER_PROP_UUIDS,
> +       ADAPTER_PROP_COD,
> +       ADAPTER_PROP_TYPE,
> +       ADAPTER_PROP_SCAN_MODE,
> +       ADAPTER_PROP_DISC_TIMEOUT,
> +       ADAPTER_PROP_SERVICE_RECORD,
> +       ADAPTER_PROP_BONDED_DEVICES
>  };

This is something I dislike right now, why are defining these and not
using directly the values defined in hal-msg.h? We should probably
have a struct with expected opcode and value e.g:
.{HAL_EV_ADAPTER_PROPS_CHANGED, BT_STATE_ON} and don't try to invent
another ID for each of those.

Luiz Augusto von Dentz