Hacker News Reader

Comment by bru | Hacker News Reader

About the patch at https://github.com/vmg/redcarpet/pull/516/files, couldn't the update simply have been the following?

    -	return Data_Wrap_Struct(klass, rb_redcarpet_rbase_mark, NULL, rndr);
    +	return Data_Wrap_Struct(klass, rb_redcarpet_rbase_mark, xfree, rndr);

View on HN

The shortest code you can get away with is not always the best. If the parameter is a structure (which I think it is here, behind the void pointer), it's a good idea to give it its own free function in case later you add additional struct members that will need freeing/cleanup in the same place. (Bonus points if it appears in roughly the same place this thing is malloc'd)

I would have been even more explicit and added a cast to the expected structure pointer type. Entirely useless, except to the human reader.

That sounds like premature generalisation to me.

I'd say use a separate free function when it needs one, which is not the case yet (and might never be.)

Call it an overreaction to a past project in which a predecessor scattered free calls over several uses, then I had to add fields.

I don't mind adding 1 useless function per complex type if it saves me those headaches even a small minority of the time, or for the next maintainer. Opinions may differ, but that's me.

I think this is more of a maintainability/readability case. Your suggestion sounds like premature optimization to me.

C is not really my area of expertise, but doesn't xfree have alternatives? like free, freeif, etc? I'd much rather have to change that later in one place than several (if there end up being more calls to xfree in the code) if at some point the implementation needs to change again.

So a from a quick googling it looks like xfree()[1] is a ruby specific thing, and should be used internally for everything allocated by ruby. It also looks like it's got the same signature as the function created for this so that should work there too. [2]

[1] http://clalance.blogspot.com/2011/01/writing-ruby-extensions... [2] http://inferior-products.com/docs/userdocs/ruby19/html/d8/d1...

...Are you sure? I was pretty sure that it gets it from the same place as xmalloc...wouldn't that be in libc?

There is no xmalloc in the libc.

Okay, interesting. I guess I've seen it before, so I didn't think it was Ruby-specific, but I also haven't had to do that kind of programming in a very super long time.

Publib then? I'm finding this when I search: http://man.cx/xfree(3) http://man.cx/publib(3)

Also, are you sure? I see xmalloc in glibc all over the internet...

edit: actually xmalloc and xfree are defined as

#define xmalloc ruby_xmalloc #define xfree ruby_xfree

That's actually fascinating. I wonder if anyone has background on this...any comments I see are in Japanese.

Replacing malloc/calloc/free etc an application-specific xmalloc/xcalloc/xfree is seemingly a fairly common pattern. There's one example in libiberty[0] (-liberty, get it?) where, for instance, the return value of malloc is checked for NULL and the program terminates if so.

[0]: http://www.delorie.com/gnu/docs/gcc/libiberty_5.html

It's fairly common to define malloc and free wrappers called xmalloc and xfree. The usual idiomatic thing to do is to have xmalloc check the return value of malloc and terminate the program if it ever fails, though this is traditionally done in application code rather than a library. I doubt libc would ever add an xmalloc function because it'd break all the code that defined its own xmalloc.

Reply on news.ycombinator.com