[PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Stefan Beller]

This was missed in 5982da9d2ce (replace-object: allow
prepare_replace_object to handle arbitrary repositories, 2018-04-11)

Technically the code works correctly as the replace_map is the same
size in different repositories, however it is hard to read. So convert
the code to the familiar pattern of dereferencing the pointer that we
assign in the sizeof itself.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 replace_object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/replace_object.c b/replace_object.c
index 246b98cd4f1..801b5c16789 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -37,7 +37,7 @@ static void prepare_replace_object(struct repository *r)
return;
 
r->objects->replace_map =
-   xmalloc(sizeof(*the_repository->objects->replace_map));
+   xmalloc(sizeof(*r->objects->replace_map));
oidmap_init(r->objects->replace_map, 0);
 
for_each_replace_ref(r, register_replace_ref, NULL);
-- 
2.17.0.582.gccdcbd54c44.dirty

Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Stefan Beller]

> Using `git grep` I see 230 instances of 'xmalloc' and 261 instances of
> 'xcalloc'. After the Coccinelle transformation, these are down to 194 and
> 190, respectively, because the rest allocate in the same line as the
> definition. It's worth thinking about the macro pattern for those cases.

Thanks for reporting the coccinelle experiment!

As we follow a strict declare before code, and we do not know if further
declarations make use of this already, e.g. given

struct foo *f = xmalloc(sizeof(*f));
struct bar b = >baz;

we cannot split up the line declaring and assigning f, but the macro
has to recreate the assignment upon declaration, for that we'd
need to have something like

ALLOCATE_TYPE(type, name);

which over complicates things IMHO.

Maybe it is worth identifying the pattern where 'f' is not used in further
declarations, such that we can make patches as

-struct foo *f = xmalloc(sizeof(*f));
+   struct foo *f;
struct baz b = 
+
+ ALLOCATE(f);
+

Thanks,
Stefan

Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Derrick Stolee]

On 5/10/2018 7:56 AM, Jeff King wrote:

On Thu, May 10, 2018 at 07:23:13PM +0900, Junio C Hamano wrote:


This one was doing

ptr = xmalloc(sizeof(*another_ptr))

and it was OK because ptr and another_ptr happened to be of the same
type.  I wonder if we are making it safer, or making it more obscure
to seasoned C programmers, if we introduced a pair of helper macros,
perhaps like these:

#define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))
#define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))

I've often wondered that, too. It's the natural endgame of the
ALLOC_ARRAY() road we've been going down.


I'll chime in that I like this idea.

Because I'm trying to learn more about Coccinelle, I added the diff 
below and ran 'make coccicheck'. It resulted in a 1000-line patch on 
'next'. I'll refrain from sending that patch series, but it's nice to 
know a "simple" transformation is possible.


Using `git grep` I see 230 instances of 'xmalloc' and 261 instances of 
'xcalloc'. After the Coccinelle transformation, these are down to 194 
and 190, respectively, because the rest allocate in the same line as the 
definition. It's worth thinking about the macro pattern for those cases.


diff --git a/contrib/coccinelle/alloc.cocci b/contrib/coccinelle/alloc.cocci
new file mode 100644
index 00..95f426c4dc
--- /dev/null
+++ b/contrib/coccinelle/alloc.cocci
@@ -0,0 +1,13 @@
+@@
+expression p;
+@@
+- p = xmalloc(sizeof(*p));
++ ALLOCATE(p);
+
+@@
+expression c;
+expression p;
+@@
+- p = xcalloc(c, sizeof(*p));
++ CALLOCATE(p,c);
+
diff --git a/git-compat-util.h b/git-compat-util.h
index f9e4c5f9bc..5398f217d7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -843,6 +843,9 @@ extern FILE *fopen_or_warn(const char *path, const 
char *mode);

  */
 #define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)

+#define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))
+#define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))
+
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), 
(alloc)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), 
st_mult(sizeof(*(x)), (alloc)))

Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Jeff King]

On Thu, May 10, 2018 at 07:23:13PM +0900, Junio C Hamano wrote:

> This one was doing
> 
>   ptr = xmalloc(sizeof(*another_ptr))
> 
> and it was OK because ptr and another_ptr happened to be of the same
> type.  I wonder if we are making it safer, or making it more obscure
> to seasoned C programmers, if we introduced a pair of helper macros,
> perhaps like these:
> 
>   #define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))
>   #define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))

I've often wondered that, too. It's the natural endgame of the
ALLOC_ARRAY() road we've been going down.

-Peff

Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Junio C Hamano]

Stefan Beller  writes:

> This was missed in 5982da9d2ce (replace-object: allow
> prepare_replace_object to handle arbitrary repositories, 2018-04-11)
>
> Technically the code works correctly as the replace_map is the same
> size in different repositories, however it is hard to read. So convert
> the code to the familiar pattern of dereferencing the pointer that we
> assign in the sizeof itself.

;-)

We say

ptr = xmalloc(sizeof(*ptr))

is better because 

ptr = xmalloc(sizeof(typeof(*ptr)))

is easy to go stale unless we actually use typeof and instead say a
concrete type like "struct oidmap".

This one was doing

ptr = xmalloc(sizeof(*another_ptr))

and it was OK because ptr and another_ptr happened to be of the same
type.  I wonder if we are making it safer, or making it more obscure
to seasoned C programmers, if we introduced a pair of helper macros,
perhaps like these:

#define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))
#define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))

The change looks obviously good.  Will queue.

Thanks.

[PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Stefan Beller]

This was missed in 5982da9d2ce (replace-object: allow
prepare_replace_object to handle arbitrary repositories, 2018-04-11)

Technically the code works correctly as the replace_map is the same
size in different repositories, however it is hard to read. So convert
the code to the familiar pattern of dereferencing the pointer that we
assign in the sizeof itself.

Signed-off-by: Stefan Beller 
---
 replace_object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/replace_object.c b/replace_object.c
index 246b98cd4f1..801b5c16789 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -37,7 +37,7 @@ static void prepare_replace_object(struct repository *r)
return;
 
r->objects->replace_map =
-   xmalloc(sizeof(*the_repository->objects->replace_map));
+   xmalloc(sizeof(*r->objects->replace_map));
oidmap_init(r->objects->replace_map, 0);
 
for_each_replace_ref(r, register_replace_ref, NULL);
-- 
2.17.0.255.g8bfb7c0704