ccan icon indicating copy to clipboard operation
ccan copied to clipboard

Bitmap tal support

Open nobrowser opened this issue 7 years ago • 5 comments

My 1st ccan patch - be gentle.

nobrowser avatar Aug 21 '17 03:08 nobrowser

Hi! Thanks, this is great... @dgibson

The main problem is we don't want ccan/bitmap to depend on tal; it's a big thing to pull in. But that just means we should have a bitmap/tal module.

Two other nitpicks:

  1. We should also keep nomenclature in spec: tal uses 'z' for zero-fill, and bitmap uses 'zero' and 'fill' for 0 and 1 resp.
  2. The "return the new allocation" is an antipattern, it's better to make bitmap_tal_resize() take the pointer to the pointer. In fact, tal_resize() does not zero the pointer on failure, so your code is actually incorrect here.

I've pushed a proposal on top of your branch, see what you think?

rustyrussell avatar Aug 22 '17 02:08 rustyrussell

That looks great, I think you should be a co-author :-) :+1:

Follow-up questions:

  1. Naming - Well, so what do to when the conventional naming differs between the two?

  2. Let's say (purely hypothetically!) that I have a new module in mind that uses bitmaps all over, including creating them dynamically. Is it ok to depend on tal then? I really hate using malloc, I think something like tal should be in libc. In fact glibc has obstacks which are poor cousins.

nobrowser avatar Aug 23 '17 03:08 nobrowser

Ian Zimmerman [email protected] writes:

That looks great, I think you should be a co-author :-) :+1:

Follow-up questions:

  1. Naming - Well, so what do to when the conventional naming differs between the two?

Naming is hard :) I'll leave this to David...

  1. Let's say (purely hypothetically!) that I have a new module in mind that uses bitmaps all over, including creating them dynamically. Is it ok to depend on tal then? I really hate using malloc, I think something like tal should be in libc. In fact glibc has obstacks which are poor cousins.

Yes, or just depend on bitmap/tal now and you'll get it by default.

I like tal, but I try not to depend on it gratuitously.

Cheers, Rusty.

rustyrussell avatar Aug 24 '17 00:08 rustyrussell

Why does the build fail?

nobrowser avatar Aug 26 '17 17:08 nobrowser

On Thu, Aug 24, 2017 at 12:26:28AM +0000, Rusty Russell wrote:

Ian Zimmerman [email protected] writes:

That looks great, I think you should be a co-author :-) :+1:

Follow-up questions:

  1. Naming - Well, so what do to when the conventional naming differs between the two?

Naming is hard :) I'll leave this to David...

Heh, thanks.

I'd say this is primarily a bitmap extension using tal, rather than a tal extension using bitmap. So, I think bitmap naming takes priority.

So I'd suggest:

bitmap_tal() bitmap_tal0() bitmap_tal1() bitmap_tal_resize0() bitmap_tal_resize1()

  1. Let's say (purely hypothetically!) that I have a new module in mind that uses bitmaps all over, including creating them dynamically. Is it ok to depend on tal then? I really hate using malloc, I think something like tal should be in libc. In fact glibc has obstacks which are poor cousins.

Yes, or just depend on bitmap/tal now and you'll get it by default.

I like tal, but I try not to depend on it gratuitously.

To expand on this: if you think tal simplifies your module enough to be worth it, go ahead and rely on it - it's your module. Just be aware that there's a tradeoff in that it makes it harder to use your module in a program (or another module) that's not tal based.

Whether it's worth it tends to depend on the complexity of your module, and how widely its applicable. e.g. I didn't want bitmap to depend on tal because it potentially has very wide applicability, and it's simple enough that tal() doesn't make things very much easier. On the other hand my rfc822 module does depend on tal, because it could potentially have many bits of allocated cached data which tal makes much easier to manage. Or it would, if I'd ever had time to get that module beyond the barest bones.

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT the other | way around! http://www.ozlabs.org/~dgibson

dgibson avatar Aug 30 '17 03:08 dgibson