Incidencia #43809

Introduce struct for requirement targets

Abrir Fecha: 2022-02-08 08:24 Última actualización: 2022-02-19 03:40

Informador:
Propietario:
Tipo:
Estado:
Cerrado
Componente:
Hito:
Prioridad:
5 - Medium
Gravedad:
5 - Medium
Resolución:
Fixed
Fichero:
2

Details

Currently, calls to is_req_active() (and accordingly, to are_reqs_active() and is_enabler_active()) are highly inscrutable, given the number of arguments (many of which are often NULL). More problematically, it's impossible to add new kinds of requirement targets without changing every call site (usually by adding another NULL at some spot in the call).

Solution: Add a new struct req_context which contains all of them and make aforementioned functions take a pointer to it. Individual call sites only need to set their relevant fields (by field name ~> better readability), so new fields can be added to the struct with less hassle.

Possible performance considerations:

  • Additional layer of indirection via pointer to req_context struct
  • Individual requirement targets don't need to be copied anymore when e.g. are_reqs_active() calls is_req_active()

Both of these are probably relatively minor.

Ticket History (3/14 Histories)

2022-02-08 08:24 Updated by: alienvalkyrie
  • New Ticket "Introduce struct for requirement targets" created
2022-02-08 08:44 Updated by: alienvalkyrie
  • Propietario Update from (Ninguno) to alienvalkyrie
Comentario

Question for the more experienced folks (wrt Freeciv code in particular, and C in general), since it doesn't seem to come up elsewhere in the codebase:

To replace a call like

  1. struct tile *ptile = NULL;
  2. if (pcity != NULL) {
  3. ptile = city_tile(pcity);
  4. }
  5. if (is_req_active(pplayer, NULL, pcity, pimprove, ptile, NULL, NULL, NULL, NULL, NULL, preq, RPT_CERTAIN)) {
  6. /* ... */
  7. }

would a better replacement be

  1. struct requirement_targets tgts;
  2. req_targets_clear(&tgts);
  3. tgts.player = pplayer;
  4. tgts.city = pcity;
  5. tgts.improvement = pimprove;
  6. if (pcity != NULL) {
  7. tgts.tile = city_tile(pcity);
  8. }
  9. if (is_req_active(&tgts, NULL, preq, RPT_CERTAIN)) {
  10. /* ... */
  11. }

or a C99 compound literal, a la

  1. struct requirement_targets tgts;
  2. struct tile *ptile = NULL;
  3. if (pcity != NULL) {
  4. ptile = city_tile(pcity);
  5. }
  6. tgts = (struct requirement_targets) {
  7. .player = pplayer,
  8. .city = pcity,
  9. .improvement = pimprove,
  10. .tile = ptile
  11. };
  12. if (is_req_active(&tgts, NULL, preq, RPT_CERTAIN)) {
  13. /* ... */
  14. }

Compound literals do show up for union types occasionally (typically universals), but I can't recall seeing them used for structs.

(The leftover NULL in the two replacements is the other_player, left separate for reasons including both actions code and future considerations.)

2022-02-08 19:48 Updated by: cazfi
Comentario

Freeciv codebase is ancient, and many conventions have never been updated. Compound literals came in C99, I think, and there were compilers that didn't support them for a long time after that. CodingStyle seems not to say anything about them. Quick googling indicates that Visual Studio (which doesn't even aim for C99 compatibility) did not support them at least in 2016.

2022-02-09 00:39 Updated by: alienvalkyrie
Comentario

Reply To cazfi

Freeciv codebase is ancient, and many conventions have never been updated. ... CodingStyle seems not to say anything about them.

Yeah, that's basically where I'm at – no mention either way, so I figured better to ask.

I think inline compound literals can make the code a lot more compact and give it a certain elegance, e.g. what I was working on just now:

  1. /**********************************************************************//**
  2. Returns TRUE if the wanted action can be done to the target city.
  3. **************************************************************************/
  4. bool is_action_possible_on_city(action_id act_id,
  5. const struct player *actor_player,
  6. const struct city* target_city)
  7. {
  8. fc_assert_ret_val_msg(ATK_CITY == action_id_get_target_kind(act_id),
  9. FALSE, "Action %s is against %s not cities",
  10. action_id_rule_name(act_id),
  11. action_target_kind_name(
  12. action_id_get_target_kind(act_id)));
  13. return is_target_possible(act_id, actor_player,
  14. &(const struct req_targets) {
  15. .player = city_owner(target_city),
  16. .city = target_city,
  17. .tile = city_tile(target_city),
  18. });
  19. }
It's pretty clear what's happening in broad strokes, but I worry about how understandable it might be to someone wanting to contribute and modify that code who's less familiar with them (or with C in general).

Also, since we're talking about this now, it might be sensible to document whatever we decide on in CodingStyle for posterity (and so that anyone encountering them knows they're called "compound literals" and can google them).

2022-02-09 00:47 Updated by: cazfi
Comentario

Reply To alienvalkyrie

to document whatever we decide on in CodingStyle

Definitely. And maybe you should already open a ticket about (the change to) CodingStyle where this discussion could continue.

2022-02-09 01:21 Updated by: alienvalkyrie
Comentario

Reply To cazfi

Reply To alienvalkyrie

to document whatever we decide on in CodingStyle

Definitely. And maybe you should already open a ticket about (the change to) CodingStyle where this discussion could continue.

Done #43826

2022-02-09 01:54 Updated by: alienvalkyrie
Comentario

There's a decision to be made regarding the struct name. So far, I've been going with req_targets (plural, which I think is more fitting than some decade-old comments which still talk about target, singular); this is fine in most cases, but in actions code, it leads to things like

  1. static bool is_enabler_active(const struct action_enabler *enabler,
  2. const struct req_targets *actor,
  3. const struct req_targets *target);
which is an overload of the word "target" that doesn't have to happen.

The alternative term I've been thinking about is "requirement evaluation context", or req_eval_context, which is a bit of a mouthful. Could be shortened to just req_context, but that sounds like it's part of the requirement; the context a requirement is defined in, rather than what it's evaluated against.

If we want to change the name and associated nomenclature, now and as part of this feature would probably be the best time and place to do so.

2022-02-14 09:23 Updated by: cazfi
Comentario

Is there a particular reason this is targeted to 3.1? As I see it, the goal of the patch is to make future development easier. This is a bit big change to stable branch where it supposedly won't help anything.

2022-02-14 10:13 Updated by: alienvalkyrie
Comentario

Reply To cazfi

Is there a particular reason this is targeted to 3.1? As I see it, the goal of the patch is to make future development easier. This is a bit big change to stable branch where it supposedly won't help anything.

No particular reason, no. As I see it, the goal of the patch is to make requirement code more readable and maintainable, which would likely help with debugging a stable branch as well.

But we can push it back if you think it's too risky – though I do have a functioning S3_1 version (regression tests looking good) that only needs another round of polishing.

2022-02-14 12:44 Updated by: alienvalkyrie
  • Resolución Update from Ninguno to Accepted
  • Details Updated
Comentario

Reply To alienvalkyrie

The alternative term I've been thinking about is "requirement evaluation context", or req_eval_context, which is a bit of a mouthful. Could be shortened to just req_context, but that sounds like it's part of the requirement; the context a requirement is defined in, rather than what it's evaluated against.

I decided to go with req_context now, as I think it's still clear enough and makes the most sense.

The attached patches depend on #43826 and #43835, the latter of which won't get merged until after S3_0 is out of code freeze.

I've also attached the S3_1 patch – the question of whether that one will get merged is still open.

2022-02-15 01:04 Updated by: cazfi
Comentario

Reply To alienvalkyrie

I've also attached the S3_1 patch – the question of whether that one will get merged is still open.

Since the work has already been done - go ahead.

2022-02-19 03:40 Updated by: alienvalkyrie
  • Estado Update from Open to Cerrado
  • Resolución Update from Accepted to Fixed

Editar

Please login to add comment to this ticket » Entrar