bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] *-map, *-omap: Allow passing NULL to search


From: Colin Watson
Subject: Re: [PATCH] *-map, *-omap: Allow passing NULL to search
Date: Mon, 11 Feb 2019 02:37:36 +0000
User-agent: NeoMutt/20170113 (1.7.2)

On Mon, Feb 11, 2019 at 02:32:11AM +0100, Bruno Haible wrote:
> Hi Colin,
> 
> > It's sometimes convenient to have a map whose values may be NULL, but in
> > that case it's a little awkward to determine whether a key exists in the
> > map: gl_map_get returns NULL for both "key not found" and "value is
> > NULL", so one needs a local variable just for the purpose of passing its
> > address to gl_map_search.
> 
> Yes. You can consider it "a little awkward", and you can get away with it
> through an inline function like this:
> 
> static bool
> gl_map_entry_exists (gl_map_t map, const void *key)
> {
>   const void *value;
>   return gl_map_search (map, key, &value);
> }

Indeed, perhaps I should have mentioned that I already have exactly such
a thing for now, differing only in the choice of function name (I prefer
"contains" over "entry_exists").

So far, the only extra supporting code I've felt the need to write
around Gnulib's container types consists of (a) equals/hash/etc.
implementations for relevant types; (b) convenience wrappers for
creating lists of strings and suchlike; (c) iteration macros; (d)
map/omap "contains" checks.  The last of those feels like it's filling
in a gap in the interface, while the others feel more naturally
dependent on what sort of things are common in a particular application.

> > Instead, allow the return pointers to be NULL, so that one can use
> > gl_map_search (map, NULL, NULL) to check existence.
> 
> What you propose is MORE awkward, for two reasons:
> 
>   1) For this code, performance is relevant.

Fair enough.  In my application layer, I very much prefer improving the
readability of the application code over a handful of cycles, so I
hadn't really been thinking of situations where a few cycles would
matter; but I recognise that these APIs are used more widely.

>   2) Allowing NULL pointers as arguments in all possible places is
>      *not* a good coding style in general. (It may be a good practice,
>      though, when you're being paid for a consulting project and your
>      code will never be maintained once you have delivered it.)

I respect your opinion as the maintainer and if you don't want to
include this that's of course fine, but please could you refrain from
implying incompetence?  It's not nice.

For what it's worth, I was taking things like gl_list_iterator_next's
handling of nodep as a partial model.  It's clear that it's sometimes
acceptable to have NULL checks in similar situations in this code
(perhaps gl_map_search is significantly more of a hot path than
gl_list_iterator_next, but that isn't obvious _a priori_), and their
presence is thus a matter of judgement of trade-offs and not simply a
matter of one's inability to maintain code competently.  One ought to be
able to discuss reasonable trade-offs without having it suggested that
one is just churning out code paid by the line.

> It can be discussed whether the public API gl_map.h and gl_omap.h
> should include functions like gl_map_entry_exists. Some people (like
> the GNOME glib authors) attempt to provide all possible variants that
> might some day be useful. Whereas I chose to provide a set of functions
> that is not so large, and omit functions that can be implemented by the
> user in a straightforward way. (gl_map_get is a notable exception to
> this rule.) gl_map_entry_exists is clearly part of those functions
> that users can implement themselves.

While it's true that I'm well able to implement it myself, I think it's
a clearly helpful thing to have and the only thing I've noticed in the
various containers APIs that really feels like an interface omission;
I'd be happy to put together a patch for that if I thought it might be
accepted.  I see that you may have a different view, though.  (Of course
gl_list doesn't need this because gl_list_search returns a node rather
than the element itself, while gl_set_search returns a bool; the
asymmetry is a bit surprising to someone learning the interface, but
understandable enough.)

Thanks,

-- 
Colin Watson                                       address@hidden



reply via email to

[Prev in Thread] Current Thread [Next in Thread]