On 2017-02-07 16:03:43 -0500, Robert Haas wrote:
> On Sun, Feb 5, 2017 at 9:48 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >> I think maybe we should rename the functions to element_allocate,
> >> element_free, and element_allocator_ctx, or something like that. The
> >> current names aren't capitalized consistently with other things in
> >> this module, and putting the word "element" in there would make it
> >> more clear what the purpose of this thing is.
> >
> > Fixed as per the suggestion
>
> Eh, not really. You changed the memory context to be called
> element_allocator_ctx, rather than changing the args passed to the
> element allocator to have that name, which is what I had in mind.
>
> I did some assorted renaming and other cosmetic improvements and committed 0002.
FWIW, I think it'd have been better to not add the new callbacks as
parameters to *_create(), but rather have them be "templatized" like the
rest of simplehash. That'd require that callback to check the context,
to know whether it should use shared memory or not, but that seems fine
to me. Right now this pushes the head of simplehash above a
cacheline...
I'm also doubtful about the naming of the default callbacks:
+/* default memory allocator function */
+static void *
+SH_DEFAULT_ALLOC(Size size, void *args)
+{
+ MemoryContext context = (MemoryContext) args;
+
+ return MemoryContextAllocExtended(context, size,
+ MCXT_ALLOC_HUGE | MCXT_ALLOC_ZERO);
+}
SH_DEFAULT_ALLOC sounds like it's a name that's one of the prefixed
names (like SH_CREATE actually becomes pagetable_create and such) - but
afaics it's not. Which afaics means that this'll generate symbol
conflicts if one translation unit uses multiple simplehash.h style
hashes. Afaics these should either be prefixed, or static inline
functions.
- Andres
