Skip to content

void *malloc(size_t) -> int malloc(void **, size_t) - #2080

Closed
ethomson wants to merge 1 commit into
libgit2:developmentfrom
ethomson:uglymalloc
Closed

void *malloc(size_t) -> int malloc(void **, size_t)#2080
ethomson wants to merge 1 commit into
libgit2:developmentfrom
ethomson:uglymalloc

Conversation

@ethomson

Copy link
Copy Markdown
Member

This was an exercise in discovering whether we could / should change the allocators to fit more with the standards we use for other things - returning an int status code and taking a pointer that will be set to the allocated memory. I'm pushing this up more for discussion, at the moment, to see if we even want to continue down this path.

Note that I did this on win32 so it's pretty likely that Travis won't even build this successfully. Also, I broke blame, so the tests there will fail miserably at the moment.

I think this was very valuable as a learning experience, as it required me to touch every allocation throughout the library. My takeaway is that switching us over to free memory in the event of an allocation failure would generally not be ridiculously burdensome.

Additional takeaways:

  1. We are good about checking the return codes from *alloc, generally. (Yay!) There were a few that we must have simply overlooked, but there was only one area that was systematically poor,.
  2. Blame needs some better error handling.
  3. git_oid__fmtz should probably exist, at least as an internal helper.

However, this didn't provide nearly the value that I had hoped. In fact, I would say that it's an overall negative since there are now more unfun compiler warnings.

Anyway. I did want to push this up for the sake of discussion, but I think that we should abandon this effort and keep malloc sane. Certainly we should go fix up the problem areas that I noticed, though.

@ethomson ethomson mentioned this pull request Jan 27, 2014
@ethomson

ethomson commented Feb 5, 2014

Copy link
Copy Markdown
Member Author

So I think that this PR provided a lot of value in terms of: more and better error checking, and some code cleanups.

However, that MSVC warns when passing additional levels on indirection to a func that takes a void ** is very disappointing. These warnings are very annoying, and casting all these would also be annoying.

I propose that we take the cleanups but keep the signature as-is. I will push up another PR when I get some time to unfactor (is that a word?) the signature changes.

If anybody objects to this strategy, please let me know!

@vmg

vmg commented Feb 7, 2014

Copy link
Copy Markdown
Member

Yes, definitely. Let's merge only the cleanups. :)

@arrbee

arrbee commented Feb 9, 2014

Copy link
Copy Markdown
Member

While I was making index changes and cleanups in #2108, I incorporated some of the types of changes here, such as more cleanup on error and changing the interface of index_entry_dup to return an error with an out parameter - in that particular case, it actually streamlined a lot of the other code to write it in that way.

@arrbee

arrbee commented Apr 24, 2014

Copy link
Copy Markdown
Member

@ethomson Have you extracted the pieces of this that you want or shall we keep it open?

@vmg

vmg commented Apr 25, 2014

Copy link
Copy Markdown
Member

Either way we decided this is not a good idea, so feel free to cherry pick what you want from the branch. Closing now!

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.

3 participants