[PATCH v5 0/2] -- Try again for GLIB v2.28 compatibility

[PATCH v5 0/2] -- Try again for GLIB v2.28 compatibility

[Brian Gix]

v4 still wasn't v2.28 compatible.  This one should be.

[PATCH v5 1/2] Add test-midi to ignore list

[Brian Gix]

---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 6257505..c0745b4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -138,6 +138,7 @@ unit/test-avdtp
 unit/test-avctp
 unit/test-avrcp
 unit/test-gatt
+unit/test-midi
 unit/test-gattrib
 unit/test-*.log
 unit/test-*.trs
-- 
1.9.1

[PATCH v5 2/2] Make unit test compatible with GLIB v2.28

[Brian Gix]

---
 unit/test-midi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/unit/test-midi.c b/unit/test-midi.c
index d318b07..3995c86 100644
--- a/unit/test-midi.c
+++ b/unit/test-midi.c
@@ -282,8 +282,12 @@ static void compare_events(const snd_seq_event_t *ev1,
 		                ev2->data.control.value);
 		break;
 	case SND_SEQ_EVENT_SYSEX:
-		g_assert_cmpmem(ev1->data.ext.ptr, ev1->data.ext.len,
-		                ev2->data.ext.ptr, ev2->data.ext.len);
+		g_assert_cmpint(ev1->data.ext.len,
+		                ==,
+		                ev2->data.ext.len);
+		g_assert(memcmp(ev1->data.ext.ptr,
+		                ev2->data.ext.ptr,
+		                ev2->data.ext.len) == 0);
 		break;
 	default:
 		g_assert_not_reached();
-- 
1.9.1

RE: [PATCH v5 2/2] Make unit test compatible with GLIB v2.28

[Gix, Brian]

H Luiz, Felipe, Szymon,

> From: Gix, Brian
> 
> ---
>  unit/test-midi.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/unit/test-midi.c b/unit/test-midi.c index d318b07..3995c86 100644
> --- a/unit/test-midi.c
> +++ b/unit/test-midi.c
> @@ -282,8 +282,12 @@ static void compare_events(const snd_seq_event_t
> *ev1,
>  		                ev2->data.control.value);
>  		break;
>  	case SND_SEQ_EVENT_SYSEX:
> -		g_assert_cmpmem(ev1->data.ext.ptr, ev1->data.ext.len,
> -		                ev2->data.ext.ptr, ev2->data.ext.len);
> +		g_assert_cmpint(ev1->data.ext.len,
> +		                ==,
> +		                ev2->data.ext.len);
> +		g_assert(memcmp(ev1->data.ext.ptr,
> +		                ev2->data.ext.ptr,
> +		                ev2->data.ext.len) == 0);
>  		break;
>  	default:
>  		g_assert_not_reached();


Here is a straightforward rework of the g_assert_cmpmem assert.

It was used only once, and both replacement asserts existed pre-v2.28

Re: [PATCH v5 2/2] Make unit test compatible with GLIB v2.28

[Felipe Ferreri Tonello]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

Hi Brian,

On 24/01/17 20:10, Gix, Brian wrote:
> 
> H Luiz, Felipe, Szymon,
> 
>> From: Gix, Brian
>>
>> ---
>>  unit/test-midi.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/unit/test-midi.c b/unit/test-midi.c index d318b07..3995c86 100644
>> --- a/unit/test-midi.c
>> +++ b/unit/test-midi.c
>> @@ -282,8 +282,12 @@ static void compare_events(const snd_seq_event_t
>> *ev1,
>>  		                ev2->data.control.value);
>>  		break;
>>  	case SND_SEQ_EVENT_SYSEX:
>> -		g_assert_cmpmem(ev1->data.ext.ptr, ev1->data.ext.len,
>> -		                ev2->data.ext.ptr, ev2->data.ext.len);
>> +		g_assert_cmpint(ev1->data.ext.len,
>> +		                ==,
>> +		                ev2->data.ext.len);
>> +		g_assert(memcmp(ev1->data.ext.ptr,
>> +		                ev2->data.ext.ptr,
>> +		                ev2->data.ext.len) == 0);
>>  		break;
>>  	default:
>>  		g_assert_not_reached();
> 
> 
> Here is a straightforward rework of the g_assert_cmpmem assert.
> 
> It was used only once, and both replacement asserts existed pre-v2.28
> 

Fine by me, but make sure you add a relevant comment in the git message,
please.

Just the title is not enough. You could write something like:

g_assert_cmpmem was added in version X of glib and since we don't want
to bump the required version of glib for BlueZ because of a unit-test,
we use g_assert_cmpint and g_assert to replace previous code.

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7177 bytes --]

RE: [PATCH v5 2/2] Make unit test compatible with GLIB v2.28

[Gix, Brian]

Hi Felipe,

On 25/1/17 Felipe Ferreri Tonello wrote:
> 
> Hi Brian,
> 
> On 24/01/17 20:10, Gix, Brian wrote:
> >
> > H Luiz, Felipe, Szymon,
> >
> >> From: Gix, Brian
> >>
> >> ---
> >>  unit/test-midi.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/unit/test-midi.c b/unit/test-midi.c index
> >> d318b07..3995c86 100644
> >> --- a/unit/test-midi.c
> >> +++ b/unit/test-midi.c
> >> @@ -282,8 +282,12 @@ static void compare_events(const
> snd_seq_event_t
> >> *ev1,
> >>  		                ev2->data.control.value);
> >>  		break;
> >>  	case SND_SEQ_EVENT_SYSEX:
> >> -		g_assert_cmpmem(ev1->data.ext.ptr, ev1->data.ext.len,
> >> -		                ev2->data.ext.ptr, ev2->data.ext.len);
> >> +		g_assert_cmpint(ev1->data.ext.len,
> >> +		                ==,
> >> +		                ev2->data.ext.len);
> >> +		g_assert(memcmp(ev1->data.ext.ptr,
> >> +		                ev2->data.ext.ptr,
> >> +		                ev2->data.ext.len) == 0);
> >>  		break;
> >>  	default:
> >>  		g_assert_not_reached();
> >
> >
> > Here is a straightforward rework of the g_assert_cmpmem assert.
> >
> > It was used only once, and both replacement asserts existed pre-v2.28
> >
> 
> Fine by me, but make sure you add a relevant comment in the git message,
> please.
> 
> Just the title is not enough. You could write something like:
> 
> g_assert_cmpmem was added in version X of glib and since we don't want to
> bump the required version of glib for BlueZ because of a unit-test, we use
> g_assert_cmpint and g_assert to replace previous code.


I can put this in the comment if people like, but it is a very straightforward change. 

You don't think the existing comment makes it clear that the change is being done to avoid up-reving GLIB?  

It is a *very* isolated change that affects only asserts, for the reason given in the headline. I think more verbiage would be excessive.

--Brian

Re: [PATCH v5 2/2] Make unit test compatible with GLIB v2.28

[Felipe Ferreri Tonello]

[-- Attachment #1: Type: text/plain, Size: 2231 bytes --]

Hi Brian,

On 25/01/17 16:43, Gix, Brian wrote:
> Hi Felipe,
> 
> On 25/1/17 Felipe Ferreri Tonello wrote:
>>
>> Hi Brian,
>>
>> On 24/01/17 20:10, Gix, Brian wrote:
>>>
>>> H Luiz, Felipe, Szymon,
>>>
>>>> From: Gix, Brian
>>>>
>>>> ---
>>>>  unit/test-midi.c | 8 ++++++--
>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/unit/test-midi.c b/unit/test-midi.c index
>>>> d318b07..3995c86 100644
>>>> --- a/unit/test-midi.c
>>>> +++ b/unit/test-midi.c
>>>> @@ -282,8 +282,12 @@ static void compare_events(const
>> snd_seq_event_t
>>>> *ev1,
>>>>  		                ev2->data.control.value);
>>>>  		break;
>>>>  	case SND_SEQ_EVENT_SYSEX:
>>>> -		g_assert_cmpmem(ev1->data.ext.ptr, ev1->data.ext.len,
>>>> -		                ev2->data.ext.ptr, ev2->data.ext.len);
>>>> +		g_assert_cmpint(ev1->data.ext.len,
>>>> +		                ==,
>>>> +		                ev2->data.ext.len);
>>>> +		g_assert(memcmp(ev1->data.ext.ptr,
>>>> +		                ev2->data.ext.ptr,
>>>> +		                ev2->data.ext.len) == 0);
>>>>  		break;
>>>>  	default:
>>>>  		g_assert_not_reached();
>>>
>>>
>>> Here is a straightforward rework of the g_assert_cmpmem assert.
>>>
>>> It was used only once, and both replacement asserts existed pre-v2.28
>>>
>>
>> Fine by me, but make sure you add a relevant comment in the git message,
>> please.
>>
>> Just the title is not enough. You could write something like:
>>
>> g_assert_cmpmem was added in version X of glib and since we don't want to
>> bump the required version of glib for BlueZ because of a unit-test, we use
>> g_assert_cmpint and g_assert to replace previous code.
> 
> 
> I can put this in the comment if people like, but it is a very straightforward change. 
> 
> You don't think the existing comment makes it clear that the change is being done to avoid up-reving GLIB?  

IMO, no.

It doesn't make clear why you decided to change the code instead of
change the glib requirement.

> 
> It is a *very* isolated change that affects only asserts, for the reason given in the headline. I think more verbiage would be excessive.

Well, I have given my opinion, but this is too small for me to push it.
It is up to Luiz to decide if he is ok with it.

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7177 bytes --]

Re: [PATCH v5 2/2] Make unit test compatible with GLIB v2.28

[Von Dentz, Luiz]

Hi,

On Wed, Jan 25, 2017 at 7:03 PM, Felipe Ferreri Tonello
<eu@felipetonello.com> wrote:
> Hi Brian,
>
> On 25/01/17 16:43, Gix, Brian wrote:
>> Hi Felipe,
>>
>> On 25/1/17 Felipe Ferreri Tonello wrote:
>>>
>>> Hi Brian,
>>>
>>> On 24/01/17 20:10, Gix, Brian wrote:
>>>>
>>>> H Luiz, Felipe, Szymon,
>>>>
>>>>> From: Gix, Brian
>>>>>
>>>>> ---
>>>>>  unit/test-midi.c | 8 ++++++--
>>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/unit/test-midi.c b/unit/test-midi.c index
>>>>> d318b07..3995c86 100644
>>>>> --- a/unit/test-midi.c
>>>>> +++ b/unit/test-midi.c
>>>>> @@ -282,8 +282,12 @@ static void compare_events(const
>>> snd_seq_event_t
>>>>> *ev1,
>>>>>                            ev2->data.control.value);
>>>>>            break;
>>>>>    case SND_SEQ_EVENT_SYSEX:
>>>>> -          g_assert_cmpmem(ev1->data.ext.ptr, ev1->data.ext.len,
>>>>> -                          ev2->data.ext.ptr, ev2->data.ext.len);
>>>>> +          g_assert_cmpint(ev1->data.ext.len,
>>>>> +                          ==,
>>>>> +                          ev2->data.ext.len);
>>>>> +          g_assert(memcmp(ev1->data.ext.ptr,
>>>>> +                          ev2->data.ext.ptr,
>>>>> +                          ev2->data.ext.len) == 0);
>>>>>            break;
>>>>>    default:
>>>>>            g_assert_not_reached();
>>>>
>>>>
>>>> Here is a straightforward rework of the g_assert_cmpmem assert.
>>>>
>>>> It was used only once, and both replacement asserts existed pre-v2.28
>>>>
>>>
>>> Fine by me, but make sure you add a relevant comment in the git message,
>>> please.
>>>
>>> Just the title is not enough. You could write something like:
>>>
>>> g_assert_cmpmem was added in version X of glib and since we don't want to
>>> bump the required version of glib for BlueZ because of a unit-test, we use
>>> g_assert_cmpint and g_assert to replace previous code.
>>
>>
>> I can put this in the comment if people like, but it is a very straightforward change.
>>
>> You don't think the existing comment makes it clear that the change is being done to avoid up-reving GLIB?
>
> IMO, no.
>
> It doesn't make clear why you decided to change the code instead of
> change the glib requirement.
>
>>
>> It is a *very* isolated change that affects only asserts, for the reason given in the headline. I think more verbiage would be excessive.
>
> Well, I have given my opinion, but this is too small for me to push it.
> It is up to Luiz to decide if he is ok with it.

The more information the better, but in this case I can added it myself.

Re: [PATCH v5 0/2] -- Try again for GLIB v2.28 compatibility

[Luiz Augusto von Dentz]

Hi Brian,

On Tue, Jan 24, 2017 at 9:52 PM, Brian Gix <brian.gix@intel.com> wrote:
>
> v4 still wasn't v2.28 compatible.  This one should be.
> --

Applied, thanks.

-- 
Luiz Augusto von Dentz