Custom memory managment

[Matt Olan]

CRoaring team,

My team at IBM is using CRoaring as part of our project and required a custom memory management solution. As there is no way to do this currently, we implemented our own which we would like to contribute back to the community. There were a large number of widespread changes required to do this, which I will outline here.

The first change is the addition of a roaring_options_t struct that can be passed in as a pointer to a new variant of externals. For example, the existing roaring_bitmap_t * roaring_bitmap_create(void) function has a new roaring_bitmap_t * roaring_bitmap_create_with_opts(roaring_options_t *options) variant that accepts an options pointer.

The roaring_options_t struct is intended to be an extendable struct that is accessible and common at all levels of the bitmap. Currently, the roaring_bitmap_t owns the lifetime of this struct, and a pointer to its instance is passed down to the roaring_array_t, and all the containers therein. This option struct is entirely optional and can be set to null.

Within roaring_options_t there is another new struct, roaring_memory_t. This contains function pointers to memory management functions (malloc, free, etc.), and a void pointer that can be used by the caller to pass down additional inputs to these functions.

Finally, each call to the built-in memory management function that existed previously in the code has been replaced with shims, for example, void* roaring_malloc(roaring_options_t*, size_t), that consume the option struct. If the options are non-null, it will use the user-provided memory function, otherwise, it will use the built-ins.

The design of this code was intended to have the least external impact possible, meaning that for the most part, all tests of the externals have remained unchanged. Changes were required on some of the lower-level test cases, for example, the ones that created and manipulated containers directly.

If these changes will be beneficial to the CRoaring project (potentially with suggested modification to better conform to existing standards and best practices in the community), we would like to make them available for feedback. Please let us know your preferred method of doing so (i.e. public fork, pull request, etc).

Thanks,

Matt Olan

[Daniel Lemire]

Thank you Matt,

Everything you describe sounds fine at a high level. Would you be able to organize a pull request?

[Matt Olan]

Daniel,

I am in the process of putting a PR together now. During development we have been using ./tools/clang-format.sh as directed in the README, however it seems this has created a large number of unrelated style changes elsewhere in the code.

Is there a specific version of clang-format we should be using? Has clang-format been run on master lately?

- Matt

[Daniel Lemire]

I do not think that clang-format is guaranteed (from version to version) to generate the same output, so even if we did apply it, you could still see a lot of unrelated changes.

That is, enforcing a style with clang-format makes more sense in a closed environment, but in an open source project with contributors from all over, to my knowledge, clang-format 

cannot really be used for that purpose. Of course, you could very specifically define the clang-format version and stick with that, but it is annoying. 

(I'd love to be proven wrong.)

We can work around such an issue easily though. 

For example, you could generate a PR request that is *just* reformatting our current main branch. Make sure that you apply clang-format exactly as you have been doing (same flags, same clang-version).

Then we do a hasty review because we know that it is just formatting. So not actual C/C++ change should be present.

Then you apply your real PR on top of that.

How does that sound?

[Paul Smith]

On Tue, 2020-12-01 at 07:28 -0800, Daniel Lemire wrote:

For example, you could generate a PR request that is *just* reformatting our current main branch. Make sure that you apply clang-format exactly as you have been doing (same flags, same clang-version).

Then we do a hasty review because we know that it is just formatting. So not actual C/C++ change should be present.

Then you apply your real PR on top of that.

As someone who's done a lot of free software development I recommend you do not do this.

All this means is that you're resetting the code to one style, which may be reset by the next person, etc.  This kind of churn is not really constructive IME.

IMO, clang-format is not fit-for-purpose outside of an enclosed dev team: its design is unfortunately not right.  If it were me I'd just delete the format definition from Git.

Instead I recommend you publish whatever coding standards you want to have as a text document, and/or tell people to make new code follow the same formatting structure as the existing code: "standard by example".  Anyone who is capable of making useful changes to CRoaring is surely capable of following the style of existing code :).

But, obviously you should do what you think is best for your project :)!

[Daniel Lemire]

I agree Paul. You actually echo my concerns and I agree 100% with your proposals. 

But it seems that they have this rather large set of changes that they want to contribute back. I am trying to be constructive.

Note that this is not "my project". This is a community driven project. I am just trying to move things along positively. I encourage everyone who disagrees with what I write to speak up and be heard. :-)

[Matt Olan]

It may be easier for me to stage my changes in a public fork and make them available for viewing there.

If my changes don't end up being something you or the community wants, then there is no point reformatting your code base before seeing them :)

Would this be a good compromise for now?

[Daniel Lemire]

I think we want your changes. This has been a highly requested feature.

The best thing would be for you to isolate your changes and just commit that.

If it is not convenient or possible, I will take the "reformat the whole project" route. I feel good

about it because I agree with everything Paul wrote. So I am aware, as you are no doubt, that 

we don't want to reformat code each time someone comes along with a pull request.

I also submit to people reading this list that there should be few hard rules. Sometimes

you want to allow things that we would not otherwise allow. We have to be practical. This

is software meant to solve real problems, not uncompromising art.

So please proceed as I suggested. I won't merge before I explain the rationale and get

others to have a look.

I do invite Paul and others to keep on providing constructive criticism.

[Matt Olan]

> The best thing would be for you to isolate your changes and just commit that.

Let me take a run at doing this before we do the reformat. I am also running into the issue that these changes are based on the v0.2.66 tag, so there are more changes to merge.

I will let you know when I have something deliverable.