Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bitmap tal support #60

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Bitmap tal support #60

wants to merge 5 commits into from

Conversation

nobrowser
Copy link

My 1st ccan patch - be gentle.

@rustyrussell
Copy link
Owner

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?

@nobrowser
Copy link
Author

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

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.

@rustyrussell
Copy link
Owner

rustyrussell commented Aug 24, 2017 via email

@nobrowser
Copy link
Author

Why does the build fail?

@dgibson
Copy link
Collaborator

dgibson commented Aug 30, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants