Freeciv

Question is what that fingerprint should be.

The option closest to what we currently have would be

 enum fc_tristate
 is_foo_in_range(const struct req_context *context,
                 const struct player *other_player,
                 enum req_range range, bool survives,
                 universals_u value)
 {
   const struct foo *pfoo = value.foo;
   /* determine if foo is in range */
 }

On the other end of the spectrum, a more compact (function-type-wise) option would be

 enum fc_tristate
 is_foo_in_range(const struct req_context *context,
                 const struct player *other_player,
                 const struct requirement *req)
 {
   fc_assert_ret_val(req->source.kind == VUT_FOO, TRI_MAYBE);
   const struct foo *pfoo = req->source.value.foo;
   enum req_range range = req->range;
   /* determine if req is active, *ignoring* req->present */
 }

If we go with the latter, it might also be sensible to rename the functions as is_foo_req_active.

Why other_player isn't part of the context? (I have a feeling that I've asked this before, or even figured it out from the code myself, but fail to remember now)

Passing requirement pointer instead of some fields from it would have the benefit that when requirement structure is changed (new fields added or whatever) we would need to change directly affected callbacks only, not all of them. And typical callback does not need to do much unpacking; if 'range' is used just once, we may as well access it directly "switch (req->range) {" instead of "enum req_range range = req->range ; switch (range) {" (difference is only in source code readability anyway, compiled code will be identical). Agree about the function naming.

Reply To cazfi

Why other_player isn't part of the context? (I have a feeling that I've asked this before, or even figured it out from the code myself, but fail to remember now)

Actions code, and future considerations. Action enablers need two of of everything anyway (actor and target), so that just uses two contexts (without the redundancy of specifying each player twice); and there is also the option to do something similar for other things with requirements in the future, turning other_player into a whole second context (which I previously wrote about here).

Reply To cazfi

Passing requirement pointer instead of some fields from it would have the benefit that when requirement structure is changed (new fields added or whatever) we would need to change directly affected callbacks only, not all of them. [ ... ] Agree about the function naming.

Yeah, it seems like the reasonable option. I'll look into doing that then.

Have you been able to run any profiling for this? To see that this really has the effect we hope (or at least; that there's no factor that we have missed that would cause this to be less efficient)

Reply To cazfi

Have you been able to run any profiling for this? To see that this really has the effect we hope (or at least; that there's no factor that we have missed that would cause this to be less efficient)

I have not. I was going to run some autogames today, both to make sure I didn't break anything, and to get a look at the turn change times.

Reply To alienvalkyrie

Reply To cazfi

Have you been able to run any profiling for this? To see that this really has the effect we hope (or at least; that there's no factor that we have missed that would cause this to be less efficient)

I have not. I was going to run some autogames today, both to make sure I didn't break anything, and to get a look at the turn change times.

Looks like nothing broke, but it doesn't seem to be consistently quicker or slower. Though I did just realize I ran it on an unoptimized (debug) build, so I'm not sure that means much.

@cazfi if you want to do some proper profiling, feel free to take over this ticket; I don't currently have the necessary builds and tools set up.

For the record, I do think this change is already worth it for the sake of abstraction and consistency, irrespective of efficiency gains; so it would still be justified to push it even without profiling data to back it up.

Reply To alienvalkyrie

For the record, I do think this change is already worth it for the sake of abstraction and consistency, irrespective of efficiency gains; so it would still be justified to push it even without profiling data to back it up.

Agreed. I'll try to do some profiling for it at some point, but there's no need to wait for those results before pushing this in.

To have sensible comparisons of two runs (with and without the patch), I would need a moment when there's no other heavy processes going on the same machine and interfering. Usually there's always a couple of builds and/or autogames going.