Patchwork [1,of,3] bdiff: use ssize_t everywhere

login
register
mail settings
Submitter Maciej Fijalkowski
Date July 14, 2016, 11:08 a.m.
Message ID <98a1290a6af89b3146d2.1468494480@brick.arcode.com>
Download mbox | patch
Permalink /patch/15851/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Maciej Fijalkowski - July 14, 2016, 11:08 a.m.
# HG changeset patch
# User Maciej Fijalkowski <fijall@gmail.com>
# Date 1468395384 -7200
#      Wed Jul 13 09:36:24 2016 +0200
# Node ID 98a1290a6af89b3146d28388c30dc24b9b7219f8
# Parent  1a1612ddd9721f196690e1fa0831764b43fc6c6e
bdiff: use ssize_t everywhere
Adrian Buehlmann - July 14, 2016, 11:59 a.m.
On 2016-07-14 13:08, Maciej Fijalkowski wrote:
> # HG changeset patch
> # User Maciej Fijalkowski <fijall@gmail.com>
> # Date 1468395384 -7200
> #      Wed Jul 13 09:36:24 2016 +0200
> # Node ID 98a1290a6af89b3146d28388c30dc24b9b7219f8
> # Parent  1a1612ddd9721f196690e1fa0831764b43fc6c6e
> bdiff: use ssize_t everywhere
                     ^^^^^^^^^^
The subject line doesn't match the patch content, as there are still
uses of Py_ssize_t in bdiff.c after applying this patch.

> diff -r 1a1612ddd972 -r 98a1290a6af8 mercurial/bdiff.c
> --- a/mercurial/bdiff.c	Mon Jul 11 13:53:35 2016 +0200
> +++ b/mercurial/bdiff.c	Wed Jul 13 09:36:24 2016 +0200
> @@ -15,12 +15,13 @@
>  #include <string.h>
>  #include <limits.h>
>  
> +#include "compat.h"
>  #include "util.h"
>  #include "bitmanipulation.h"
>  
>  struct line {
>  	int hash, n, e;
> -	Py_ssize_t len;
> +	ssize_t len;
>  	const char *l;
>  };
>  
> @@ -34,7 +35,7 @@
>  	struct hunk *next;
>  };
>  
> -static int splitlines(const char *a, Py_ssize_t len, struct line **lr)
> +static int splitlines(const char *a, ssize_t len, struct line **lr)
>  {
>  	unsigned hash;
>  	int i;
Maciej Fijalkowski - July 14, 2016, 1:51 p.m.
On Thu, Jul 14, 2016 at 1:59 PM, Adrian Buehlmann <adrian@cadifra.com> wrote:
> On 2016-07-14 13:08, Maciej Fijalkowski wrote:
>> # HG changeset patch
>> # User Maciej Fijalkowski <fijall@gmail.com>
>> # Date 1468395384 -7200
>> #      Wed Jul 13 09:36:24 2016 +0200
>> # Node ID 98a1290a6af89b3146d28388c30dc24b9b7219f8
>> # Parent  1a1612ddd9721f196690e1fa0831764b43fc6c6e
>> bdiff: use ssize_t everywhere
>                      ^^^^^^^^^^
> The subject line doesn't match the patch content, as there are still
> uses of Py_ssize_t in bdiff.c after applying this patch.

It uses ssize_t in places that are later copied to a different file. I
did not want to touch lines that are irrelevant
as to not introduce changes that are completely unnecessary. What
would you want instead?

>
>> diff -r 1a1612ddd972 -r 98a1290a6af8 mercurial/bdiff.c
>> --- a/mercurial/bdiff.c       Mon Jul 11 13:53:35 2016 +0200
>> +++ b/mercurial/bdiff.c       Wed Jul 13 09:36:24 2016 +0200
>> @@ -15,12 +15,13 @@
>>  #include <string.h>
>>  #include <limits.h>
>>
>> +#include "compat.h"
>>  #include "util.h"
>>  #include "bitmanipulation.h"
>>
>>  struct line {
>>       int hash, n, e;
>> -     Py_ssize_t len;
>> +     ssize_t len;
>>       const char *l;
>>  };
>>
>> @@ -34,7 +35,7 @@
>>       struct hunk *next;
>>  };
>>
>> -static int splitlines(const char *a, Py_ssize_t len, struct line **lr)
>> +static int splitlines(const char *a, ssize_t len, struct line **lr)
>>  {
>>       unsigned hash;
>>       int i;
Yuya Nishihara - July 14, 2016, 3:02 p.m.
On Thu, 14 Jul 2016 15:51:22 +0200, Maciej Fijalkowski wrote:
> On Thu, Jul 14, 2016 at 1:59 PM, Adrian Buehlmann <adrian@cadifra.com> wrote:
> > On 2016-07-14 13:08, Maciej Fijalkowski wrote:  
> >> # HG changeset patch
> >> # User Maciej Fijalkowski <fijall@gmail.com>
> >> # Date 1468395384 -7200
> >> #      Wed Jul 13 09:36:24 2016 +0200
> >> # Node ID 98a1290a6af89b3146d28388c30dc24b9b7219f8
> >> # Parent  1a1612ddd9721f196690e1fa0831764b43fc6c6e
> >> bdiff: use ssize_t everywhere  
> >                      ^^^^^^^^^^
> > The subject line doesn't match the patch content, as there are still
> > uses of Py_ssize_t in bdiff.c after applying this patch.  
> 
> It uses ssize_t in places that are later copied to a different file. I
> did not want to touch lines that are irrelevant
> as to not introduce changes that are completely unnecessary. What
> would you want instead?

How about "use ssize_t where functions will be cpy-agnostic" ?

I'll review the series tomorrow. Thanks for splitting patches.
Adrian Buehlmann - July 15, 2016, 6:38 a.m.
On 2016-07-14 15:51, Maciej Fijalkowski wrote:
> On Thu, Jul 14, 2016 at 1:59 PM, Adrian Buehlmann <adrian@cadifra.com> wrote:
>> On 2016-07-14 13:08, Maciej Fijalkowski wrote:
>>> # HG changeset patch
>>> # User Maciej Fijalkowski <fijall@gmail.com>
>>> # Date 1468395384 -7200
>>> #      Wed Jul 13 09:36:24 2016 +0200
>>> # Node ID 98a1290a6af89b3146d28388c30dc24b9b7219f8
>>> # Parent  1a1612ddd9721f196690e1fa0831764b43fc6c6e
>>> bdiff: use ssize_t everywhere
>>                      ^^^^^^^^^^
>> The subject line doesn't match the patch content, as there are still
>> uses of Py_ssize_t in bdiff.c after applying this patch.
> 
> It uses ssize_t in places that are later copied to a different file. I
> did not want to touch lines that are irrelevant
> as to not introduce changes that are completely unnecessary. What
> would you want instead?

Perhaps, it would be less confusing if you would reorganize your patches
and do the file split first and then replace Py_ssize_t in the split part?
Jun Wu - July 15, 2016, 11:28 a.m.
The refactoring approach has been discussed at

https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-July/085944.html

Rename-first is cleaner because you won't need Python.h and other possible
CPython bloat code when doing the move. It has been going well, although the
title could be less ambiguous.

Excerpts from Adrian Buehlmann's message of 2016-07-15 08:38:20 +0200:
> On 2016-07-14 15:51, Maciej Fijalkowski wrote:
> > On Thu, Jul 14, 2016 at 1:59 PM, Adrian Buehlmann <adrian@cadifra.com> wrote:
> >> On 2016-07-14 13:08, Maciej Fijalkowski wrote:
> >>> # HG changeset patch
> >>> # User Maciej Fijalkowski <fijall@gmail.com>
> >>> # Date 1468395384 -7200
> >>> #      Wed Jul 13 09:36:24 2016 +0200
> >>> # Node ID 98a1290a6af89b3146d28388c30dc24b9b7219f8
> >>> # Parent  1a1612ddd9721f196690e1fa0831764b43fc6c6e
> >>> bdiff: use ssize_t everywhere
> >>                      ^^^^^^^^^^
> >> The subject line doesn't match the patch content, as there are still
> >> uses of Py_ssize_t in bdiff.c after applying this patch.
> > 
> > It uses ssize_t in places that are later copied to a different file. I
> > did not want to touch lines that are irrelevant
> > as to not introduce changes that are completely unnecessary. What
> > would you want instead?
> 
> Perhaps, it would be less confusing if you would reorganize your patches
> and do the file split first and then replace Py_ssize_t in the split part?

Patch

diff -r 1a1612ddd972 -r 98a1290a6af8 mercurial/bdiff.c
--- a/mercurial/bdiff.c	Mon Jul 11 13:53:35 2016 +0200
+++ b/mercurial/bdiff.c	Wed Jul 13 09:36:24 2016 +0200
@@ -15,12 +15,13 @@ 
 #include <string.h>
 #include <limits.h>
 
+#include "compat.h"
 #include "util.h"
 #include "bitmanipulation.h"
 
 struct line {
 	int hash, n, e;
-	Py_ssize_t len;
+	ssize_t len;
 	const char *l;
 };
 
@@ -34,7 +35,7 @@ 
 	struct hunk *next;
 };
 
-static int splitlines(const char *a, Py_ssize_t len, struct line **lr)
+static int splitlines(const char *a, ssize_t len, struct line **lr)
 {
 	unsigned hash;
 	int i;