Re: [AD] resource path functions [was: [patch] new configuration variable: resource_path]

[Daniel Schlyder <daniel@xxxxxxxxxx>]

Grzegorz Adam Hankiewicz:
> The argument about having one function to add and another to remove
> doesn't hold much strength when you also use the add function to modify a
> currently existant path.

I think this is a minor issue compared to having remove and add
functionality in the same function, but I see your point.

> The problem really is: from the two parameters (path and priority), which
> one "drives" the other one around?

> The way you have considered the API, the main parameter seems to be the
> path. That's why you have functions to add or remove paths, the priority
> seems like an unimportant attribute that just dangles around and is used
> for the not so sexy operation of sorting.

> From the POV of API design, the problem is that logical actions have to be
> applied on the path, and that's why ideally you would need three different
> functions for the three different basic operations.

Thanks for your detailed response! Some of your arguments against my patch
are invalid because they argue against a set_allegro_resource_path() that
the patch did not implement. Anyway, I see now how having 'priority' be the
first argument would make it more natural to have just one API function and
simplify implementation somewhat, so attached is a new patch that I hope you
will find more acceptable.

> You could of course still use the vague set_allegro_resource_path() I
> suggested, but that would mean that the path always has to be valid. In
> order to remove a path with this idea, you would have to restrict the
> range of valid priorities, say, negative numbers mean you want to remove
> the path.

This has nothing to do with my patch, of course.

> You would also end up with users who would like to attach the same
> priority to different paths.

> What do you do then? Sort alphabetically the paths with same priority?

My patch handled this by adding new paths with priority equal to existing
ones after them in the list. Isn't that sufficient?

> And what do you do in platforms where you can have case insensitive paths?
> Or what do you do if you have the same path expressed as a different
> string (/home/user/dir vs. ~/dir)? From the user's point of view both
> paths are the same, but that would mean the implementation has to resolve
> the true or full path before knowing if the string being passed
> corresponds to one path already stored.... IMO all that is just a can of
> worms you open for the implementation and which is error prone to the
> user.

Ok, good point.

> On the other hand, if you consider the priority to be the main parameter
> and the path to be the "dangling attribute", things are much easier.

> First of all, there is no restriction in the range of valid priorities you
> can pass to the function, because the full range of integers can be used.
> No weird decisions like negative values meaning this or that. The only
> thing you have to define if bigger numbers have more or less priority than
> smaller numbers.

Here you are presuming my patch implemented set_allegro_resource_path(),
which it did not.

> Second, adding and removing paths become naturally exclusive given that an
> invalid path naturally falls to be NULL in the C language. Again, anything
> else is considered as a valid path. No value restrictions here either.

> Third, you disallow having paths with the same priority. This could be
> troublesome if Allegro internally wanted to use some of this and allocate
> paths for itself, but that's not the case. For the user, the options are
> simple: if you allocate a priority, it's used. To add another one you need
> a different number. It solves the question of sorting paths with the same
> priority number.

Ok, I agree, this is nice.

> Fourth, Allegro can be "passive" or "lazy" in terms of detecting incorrect
> parameters. All integer values are OK as priority, and setting a priority
> to NULL would be a no-op if the priority is not used. No ASSERTs for the
> parameters. Plus you don't need to check the validity of the path as it is
> being added, because you can treat it as an opaque string which will be
> used much much later, whenever a resource is looked for.

> Finally, you only need a single API entry point.

Valid point, but I'd just like to point out that here you are presuming my
patch haven't implemented set_allegro_resource_path(), but some of your
arguments above depend on my patch having done just that. Which is it? :)

> Even though you miss the logical actions in the function name, I think
> it's a win situation for the user. The different operations you can do
> with the same function are clearly defined by the parameters themselves.
> There's no way you could interpret them in the incorrect way. In fact, I
> think some users would easily discover without documentation how to use
> the function just by looking at the prototype.

Ok, you've convinced me.

> About remove_all_allegro_resource_paths(). I would prefer this to be
> internal to the library, called by allegro_exit. Even the sloppiest user
> could do the following lazy loop to remove all his registered entries:

>   for (i = START_RANGE; i < END_RANGE; i++)
>      set_allegro_resource_path(i, NULL);

> Having a library function as convenience to avoid such a simple loop
> doesn't feel reasonable to me.

Ok, agreed. I just couldn't make up my mind. I've made it static in
src/file.c and renamed it to destroy_resource_path_list().

> For these reasons I reject your patch, unless you provide arguments to
> counter the disadvantages I have mentioned.

I hope you'll find the new one more to your liking.

--
Daniel Schlyder
http://bitblaze.com/

Attachment: resource_path_functions-2a.diff
Description: Binary data