v4 still wasn't v2.28 compatible. This one should be.
Hi Brian,
On Tue, Jan 24, 2017 at 9:52 PM, Brian Gix <[email protected]> wrote:
>
> v4 still wasn't v2.28 compatible. This one should be.
> --
Applied, thanks.
--
Luiz Augusto von Dentz
Hi,
On Wed, Jan 25, 2017 at 7:03 PM, Felipe Ferreri Tonello
<[email protected]> 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.
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
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
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
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
---
.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
---
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