Freeciv

I read patch and I do not see if ruleset loader routine check there is too many counters in ruleset (array have fixed length). I will look inside on my free time and try to correct this.

OK: I opened up my IDE and there is check on line 1416.

I check coding-style. Few correction/fixes only. All good for now.

Apply "Adds missing changes" and next "Patch only for testing purposes"

Apply: Code of patch was adjusted to its responsibles I think, I will end this patch (at worst case) at end of next week. Sorry. And of course, responsibility instead of responsibles.

@cazi: Can you look?

This looks mostly good already. Couple of things I noticed on quick reading:

- int num = nval; /* No "size_t" to printf */ -> Please use the SIZE_T_PRINTF macro instead of tricks like this (which may fail itself on some environments as size_t > int)
- In an error situation you section_list_destroy() twice - once when you detect the error, but still also in the end of handling
- Don't name variable outside macros, or similar constructs, from using namespace '*_'. You have 'curr_' named like that for no reason (and potentially conflicting with macro calls we later add there)
- Matter of taste, but I wouldn't have default value (of 1) for checkpoint, but to make it mandatory field instead

What you need to add:
- Saving the rules in tools/ruleutil/rulesave.c
- Documentation in rulesets (but we probably want to agree on one copy first, before having it copied to all supplied rulesets

Reply To cazfi

This looks mostly good already. Couple of things I noticed on quick reading: - int num = nval; /* No "size_t" to printf */ -> Please use the SIZE_T_PRINTF macro instead of tricks like this (which may fail itself on some environments as size_t > int)
- In an error situation you section_list_destroy() twice - once when you detect the error, but still also in the end of handling
- Don't name variable outside macros, or similar constructs, from using namespace '*_'. You have 'curr_' named like that for no reason (and potentially conflicting with macro calls we later add there)
- Matter of taste, but I wouldn't have default value (of 1) for checkpoint, but to make it mandatory field instead
--- What you need to add:
- Saving the rules in tools/ruleutil/rulesave.c
- Documentation in rulesets (but we probably want to agree on one copy first, before having it copied to all supplied rulesets

Please ignore:

About (please ignore for this ticket, but read my comment).

It repairs intended change to counter_behaviour specenum, which makes default value not be treated by any part of freeciv (default int/enum is zero and by skipping value0 we solve many thinks). But... In future, I set MAX_COUNTERS to COUNTER_BEHAVIOUR_LAST, so it introducing bugs. I must review my changes. Sorry I miss this :-( . It also introduces remove_counter routine. If you wish, I will split this patch, but you must reopen related ticket. Any other way to solve this?

About:

It is change related to this ticket. I untested it, but implementation is very simple and I do not see any possible problems.

Reply To cazfi

This looks mostly good already. Couple of things I noticed on quick reading: - int num = nval; /* No "size_t" to printf */ -> Please use the SIZE_T_PRINTF macro instead of tricks like this (which may fail itself on some environments as size_t > int)
- In an error situation you section_list_destroy() twice - once when you detect the error, but still also in the end of handling
- Don't name variable outside macros, or similar constructs, from using namespace '*_'. You have 'curr_' named like that for no reason (and potentially conflicting with macro calls we later add there)
- Matter of taste, but I wouldn't have default value (of 1) for checkpoint, but to make it mandatory field instead
--- What you need to add:
- Saving the rules in tools/ruleutil/rulesave.c
- Documentation in rulesets (but we probably want to agree on one copy first, before having it copied to all supplied rulesets

See above post. I added also documentation.

Sorry I've had no time to look at this.

The ruledit conters support will definitely be needed. Good point. I don't think we have a ticket for that yet. Please open one, and keep things related to that one there.

In general there's too much stuff unrelated to the ticket at hand. Please try to keep things organized. It would really save me time if I wouldn't need to figure out what's relevant and what is not etc.

Can you create a single patch that has the content that is supposed to get pushed in in this ticket?

For the ruleset documentation comment, please follow the format that other similar section type documentation comments use.

Reply To cazfi

Sorry I've had no time to look at this. The ruledit conters support will definitely be needed. Good point. I don't think we have a ticket for that yet. Please open one, and keep things related to that one there. In general there's too much stuff unrelated to the ticket at hand. Please try to keep things organized. It would really save me time if I wouldn't need to figure out what's relevant and what is not etc. Can you create a single patch that has the content that is supposed to get pushed in in this ticket? For the ruleset documentation comment, please follow the format that other similar section type documentation comments use.

Partially done.

Reply To cazfi

Sorry I've had no time to look at this. The ruledit conters support will definitely be needed. Good point. I don't think we have a ticket for that yet. Please open one, and keep things related to that one there. In general there's too much stuff unrelated to the ticket at hand. Please try to keep things organized. It would really save me time if I wouldn't need to figure out what's relevant and what is not etc. Can you create a single patch that has the content that is supposed to get pushed in in this ticket? For the ruleset documentation comment, please follow the format that other similar section type documentation comments use.

0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch(7KB)

Two above are prepared to apply - apply only these two to fulfilled ticket requirement.

Reply To lachu

0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch(7KB) Apply only this patch. It do not contains documentation and ruleset parts 0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch(7KB) Documentation correction

These seem like you've uploaded the same patch twice - there's no documentation patch.

- As you remove initialization from countersMAX_COUNTERS, you should declare it just "static struct counter countersMAX_COUNTERS;" - remove "{}" too
- From the rulesave it seems that you're placing these counter sections between "trade" (specifically: trade.goods_selection") and "goods" sections. Better to not split those two apart, but maybe place this (as a new feature) after "clauses"

Reply To cazfi

Reply To lachu

Apply these patches:

2022-06-10 23:19 Updated by: lachu

2022-06-10 23:32 Updated by: lachu

And:

I made style changes to global array of each counter class definition, but it is probably not related to this ticket. Put patch in this topic?

Can you make this just two patches; one for all the code changes and one for the documentation proposal? It seems that one is again expected to apply number of these patches (and it's not clear which ones and in which order) to get the correct "current" state of you work. I'm sure it would be possible to decipher from the comments and the patches themselves, but I'd prefer not to spend too much of my time on it (I have limited time for dozens of patches in my queue & all the other tasks I have)

Apply All the related code patch (Loading + Saving Ruleset patch).

EDIT: Tested after solve problem with server/client version mismatch. Working.

Looks good. Just two minor style issues

- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"
- Couple of "save_default_int(... path,"xxx")" lack the space after comma

While we ideally would want to keep the ruleset documentation in sync with the code, this is getting too slow. Maybe we should split this ticket to the code change (almost ready) and to the documentation update to be handled after this?

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"
- Couple of "save_default_int(... path,"xxx")" lack the space after comma
While we ideally would want to keep the ruleset documentation in sync with the code, this is getting too slow. Maybe we should split this ticket to the code change (almost ready) and to the documentation update to be handled after this?

I think better approach is move to another target milestone/version. I have also other task and do not known how fast could I deliver changes :-( .

Reply To (Anonymous)

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"
- Couple of "save_default_int(... path,"xxx")" lack the space after comma
While we ideally would want to keep the ruleset documentation in sync with the code, this is getting too slow. Maybe we should split this ticket to the code change (almost ready) and to the documentation update to be handled after this?

I think better approach is move to another target milestone/version. I have also other task and do not known how fast could I deliver changes :-( .

This comment was mine. I have foreign language's trouble, so writing documentation is not my strong side.

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"
- Couple of "save_default_int(... path,"xxx")" lack the space after comma
While we ideally would want to keep the ruleset documentation in sync with the code, this is getting too slow. Maybe we should split this ticket to the code change (almost ready) and to the documentation update to be handled after this?

Just apply this and the doc.

Reply To lachu

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"

There seems to be change to the wrong direction in attach_city_counter()! load_ruleset_game() has one instance of '++curr' that is not fixed.

Spit the documentation update -> #45028

Reply To cazfi

Reply To lachu

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"

There seems to be change to the wrong direction in attach_city_counter()! load_ruleset_game() has one instance of '++curr' that is not fixed. Spit the documentation update -> #45028

Patch send and it has name "In response to: 2022-07-06 08:49 Updated by: cazfi"

I think this is now ready to be merged (starting review period).

It's ok for now, but in the future it might make more sense to set counter.index in attach_city_counter() than in ruleset.c. I think this needs to be changed latest when we provide client side with the counters information.