Introduce struct for requirement targets
alienvalkyrie alienvalkyrie 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
would a better replacement be
or a C99 compound literal, a la
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.)
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.
alienvalkyrie 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:
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).
- /**********************************************************************//**
- Returns TRUE if the wanted action can be done to the target city.
- **************************************************************************/
- bool is_action_possible_on_city(action_id act_id,
- const struct player *actor_player,
- const struct city* target_city)
- {
- fc_assert_ret_val_msg(ATK_CITY == action_id_get_target_kind(act_id),
- FALSE, "Action %s is against %s not cities",
- action_id_rule_name(act_id),
- action_target_kind_name(
- action_id_get_target_kind(act_id)));
- return is_target_possible(act_id, actor_player,
- &(const struct req_targets) {
- .player = city_owner(target_city),
- .city = target_city,
- .tile = city_tile(target_city),
- });
- }
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).
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.
alienvalkyrie 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
alienvalkyrie 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
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.
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.
alienvalkyrie 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.
alienvalkyrie alienvalkyrie 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.
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.
alienvalkyrie 
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:
Both of these are probably relatively minor.