Freeciv

I created metaticket listing all counters related work in osdn: Ticket #41119

Sorry for messing in this thread. I send 0002-Added-missing-pieces-for-counters-implementation.patch, which implements very basics.

- counter.index is documented as "/* index in specific (city/player/world) array */" - your patch assumes it to be index in global array (actually; 'id' is an index in global array)

. The first "/***..." lines of function headers are too short and lack "*//**" required by doxygen in the end

- When both work equally well, prefer 'i++' over '++i'

- "fc_casestrcmp(name,countersi .rule_name))" -> "fc_casestrcmp(name, countersi.rule_name))" (one space added, another removed)

I don't known how to distinguish array of index is related. I only copy your's function prototypes. I read some book about C in past and it explains you cannot substract two pointers if it don't belongs to the same array. Behavior of C language in this case is unspecified.

New version of https://www.hostedredmine.com/issues/876776 adds enum counter_target, and I updated counter_by_index() prototype here.

Reply To cazfi

New version of https://www.hostedredmine.com/issues/876776 adds enum counter_target, and I updated counter_by_index() prototype here.

Will you make a new version based on that?

Yes. In this week (I think on weekend), I should use more time on programming.

I added the work related to add counter_type above my work (git apply won't apply patch, so I made this by hand). It makes sense, because counter_by_index was changes to accept target of counter (for example).

Can you make this one patch that applies on top of current git HEAD?

I don't known VCS (like git) much. Any suggestions? I will search on net, but if you can sent some private message with link or do it by yourselves?

Reply To lachu

I don't known VCS (like git) much. Any suggestions? I will search on net, but if you can sent some private message with link or do it by yourselves?

How I do it:

For small deviations: git checkout branch_to_put_it_on git am /tmp/patch_name.patch patch -p1 < /tmp/patch_name.patch # handle what wasn't merge by hand git add filename.c # as you are done git am --continue

For medium deviations: git checkout branch_to_put_it_on git am /tmp/patch_name.patch patch -mp1 < /tmp/patch_name.patch # tries to merge # handle what wasn't merge by hand git add filename.c # as you are done git am --continue

For large deviations: git checkout branch_to_put_it_on git am /tmp/patch_name.patch cp /the/files/that/wont/merge.c the/files/in/this/branch.c # a lot of manual porting in this step git add filename.c # as you are done git am --continue

I probably have some spelling eror in my commands so research them your self before you use them.

I switch to master, apply changes (without 5 patch, because 5. patch was applied on master) and squash changes.

So, Prepare patches related to next ticket meanwhile you test this changes?

- counter_by_index() is missing the target parameter. Did you forgot to squash in your latest changes?

- Add comment telling the directory "/* utility */" before fcintl.h include

- Many of the asserts seem reversed

You can work on multiple tickets at the same time. Only you need to be able to address issues found in earlier tickets, so you need to learn how to use git. Some instructions in http://www.freeciv.org/wiki/How_to_Contribute#How_to_create_patch_file_with_Git

Start adding patches from 0001-All-changes-listed-below-are-related-to-counters-imp.patch to 0003-Added-missing-reference-to-utility-in-counters.c-fil.patch .

Can Sveinung take over as the maintainer handling the counters work? My hands are full with the preparations of the 3.0.0-beta1 release.

lachu: https://git-scm.com/book/en/v2 is a great resource for learning git

Superficial feed back just so you know that I have started: 1) I assume that 0001-All-changes-listed-below-are-related-to-counters-imp.patch 0002-Assertion-repair.patch and 0003-Added-missing-reference-to-utility-in-counters.c-fil.patch is supposed to be one patch. Squash them. (See git rebase --help You want git rebase -i and then using f or s to merge a patch into the patch above it) 2) Line length. See the coding style.

Last patch (looking at bottom) is the latest version. I squash commits and repair too-long-line bug.

Sending the feed back I already have written. I hope to implement something on top of your patch to test it.

Git gives me white space warning when I apply your patch. Don't add lines that are 100% white space.

Why the blank line at the start of a block? if (TRUE) {

}

"After variables are declared, there should be an empty line before the rest of the function body."

Read the coding style. If your text editor formats code for you set it to GNU. It is close enough to Freeciv's style to make coding style fixes less work.

I will need a better commit message when I merge this. Do you want me to write it so you have an example to copy for your next patch?

Reply To kvilhaugsvik

Sending the feed back I already have written. I hope to implement something on top of your patch to test it.

For that I need the ability to put a counter value in a city. I think that is #41116 Have you done it? If not: do you want me to do it? I'm fine with doing it myself. I'm also fine with you doing it before this patch is committed.

I will need a better commit message when I merge this. Do you want me to write it so you have an example to copy for your next patch?

My offer to write it for you was probably a mistake. You need to learn. Guidelines: The first line should be short and describe what you patch does. A soft limit is 50 chars lonng. People see this - not the full message - when they consider if they need to read the patch. The first line should be followed by a blank line, followed by a description. The description should be closer to "why" than "how". It can mention why a particular way of achieving the "why" was selected. It should contain enough "how" to let me know if something is wrong. The last line is See tracker #issuenumber so here See osdn #41115

Oh, and a warning: You are playing on a higher difficulty level than I believed you were. I found some old notes. I had the idea about counters when looking at how to make Airlift enabler controlled, something that was done in time for 3.0. This means that I have been working on my own idea of counters, forgotten about it, gotten the "new idea" again, worked on it some more and then again forgotten it - for years. This makes me a lot less open minded than I imagined I would be. So try to be extremely clear about what your code is intended to do. If not I'm afraid I'll misinterpret you and substitute what you are trying to say with my own idea. I'll try to read up on the discussion you had with Marko in the issue tracker but beware that I still may get you completely wrong.

if (countersi.id == id) {

Won't countersid always be the counter with the id of i?

Reply To kvilhaugsvik

I think that is #41116

I did a lot of thinking when setting the order of tickets listed in meta-ticket #41119 so that later tickets build on earlier ones and everything needed for proper functioning is always present (savegame handling has to come relatively early so that save+load cycle does not alter anything that matters), and not have dependencies the wrong way.

It's possible that I've missed something, but I think generally you should follow that order. And yes, #41116 would be next.

Reply To cazfi

I did a lot of thinking when setting the order of tickets listed in meta-ticket #41119 so that later tickets build on earlier ones and everything needed for proper functioning is always present (savegame handling has to come relatively early so that save+load cycle does not alter anything that matters), and not have dependencies the wrong way.

Thank you for the link, Marko!

Sorry for delay. I will resume developing in near week. If somebody would like to write some code on top of mine, it's fair. Especially savegame-related code or network code. That is two domains I'm not too good.

Reply To lachu

Sorry for delay.

No problem. Remember who you are talking to. I just committed a patch that I procrastinated on completing for almost 10 years.

If somebody would like to write some code on top of mine, it's fair.

Great! The reason why I would prefer to have more than just this patch ready before I push it is that I'm not the greatest at spotting details. If I can get a patch series that forces the compiler to compile everything rather than skipping it as unused as an optimization the compiler will detect some of the details you may have gotten wrong and my testing will detect others.

Reply To kvilhaugsvik

if (countersi.id == id) {

Won't countersid always be the counter with the id of i?

In current implementation, yes. We have only one counter type currently. It have kind/range city. Each city will contains a single value of this counter. In future, ruleset could define counters, so there could exist many counters of this kind (city range), player range, unit range, etc.

So static counters struct at top of counters.c contains only one counter currently and type do not mean kind/range. It mean number of counter in array. In future, we could remove enum counter_type and use ruleset defined counter types.

Did you receive message about update: https://osdn.net/projects/freeciv/ticket/41116 ?

Reply To lachu

In current implementation, yes. We have only one counter type currently. It have kind/range city. Each city will contains a single value of this counter. In future, ruleset could define counters, so there could exist many counters of this kind (city range), player range, unit range, etc.

Exactly what is "id" supposed to mean? Counter type? Global counter id? City counter id?

Reply To kvilhaugsvik

Reply To lachu

In current implementation, yes. We have only one counter type currently. It have kind/range city. Each city will contains a single value of this counter. In future, ruleset could define counters, so there could exist many counters of this kind (city range), player range, unit range, etc.

Exactly what is "id" supposed to mean? Counter type? Global counter id? City counter id?

int id; /* id in global counters array */

So, global counter id. There is also "int index; /* index in specific (city/player/world) array */" and "enum counter_type type;"

Reply To cazfi

So, global counter id.

That is my interpretation too but in that case it should be possible to just look it up in the array rather than looping. So I wonder what Lachu's interpretation of 'id' is.

Creating this patch takes too long. I didn't create this structure by my own. As I now remind, there will exist separate global arrays for describing counters of city. It will contains pointers to array of all counters. Id will point the index in this main, global array.

global_array { COUNT_CITY_OWNED = { .id = 0 }, COUNT_UNIT_LIVE = { .id = 1} }

// Descriptions

city_array { &COUNT_CITY_OWNED }

unit_array { &COUNT_UNIT_LIVE }

And yet:

Unit_1 { counter_values = { 5 /* Unit Live for 5 years */ } }

City_1 { counter_values = { 5 /* City is conquered/created five turns ago */ } }

Sorry for misunderstanding.

Reply To lachu

Creating this patch takes too long.

I agree. A clarification to make sure it doesn't take longer: All I ask is that you optimize by looking up the counter by id in the table you can look up by global id rather than looping over it and that you fix the coding style. You can add an assertion that the counter you looked up has the id in case not checking that makes you uncomfortable.

That is true I can return counter by index from global array :-) . I also do some code formatting fixes.

One more thing: OSDN doesn't tell me when you upload a patch. I discovered this one now.

Reply To kvilhaugsvik

Reply To kvilhaugsvik

Sending the feed back I already have written. I hope to implement something on top of your patch to test it.

For that I need the ability to put a counter value in a city. I think that is #41116 Have you done it? If not: do you want me to do it? I'm fine with doing it myself. I'm also fine with you doing it before this patch is committed.

I think 41119 is nearly ready.

What patch is 0008-Code-style-changes-to-match-Freeciv-code-rules.patch supposed to apply on top of?

Do you know how to merge changes so they come in one patch?

Reply To kvilhaugsvik

What patch is 0008-Code-style-changes-to-match-Freeciv-code-rules.patch supposed to apply on top of? Do you know how to merge changes so they come in one patch?\

Do you mean squashing changes? I known how to do it. I will prepare single path and send here, but firstly I will apply my changes at top master/trunk.

Reply To (Anonymous)

Do you mean squashing changes?

Yes.

I known how to do it. I will prepare single path and send here, but firstly I will apply my changes at top master/trunk.

Thank you. A single patch for each issue. (One for this, one for #41116)

You can apply my patch on latest master. You may apply one single RC- file.

@kvilhaugsvik : Hi. What is about my patches? I realized that you can turn on monitoring of this topic in top-right corner. I wrote about it, because someone told it is impossible or he/she do not monitor topic for other reason.

In counter_by_rule_name():

fc_assert_ret_val(NULL == name, NULL);

asserts that the value is NULL. You should assert that it *isn't* NULL.

Reply To lachu

@kvilhaugsvik : Hi. What is about my patches?

Sorry about the delay.

I realized that you can turn on monitoring of this topic in top-right corner.

I get notified on new comments. I don't get notified on a new patch file upload without a comment. This time it was my fault.

Sorry for delay. I read last comment and think my patches are in review. I missing comment describing my errors.

Reply To kvilhaugsvik

Reply To lachu

@kvilhaugsvik : Hi. What is about my patches?

Sorry about the delay.

I realized that you can turn on monitoring of this topic in top-right corner.

I get notified on new comments. I don't get notified on a new patch file upload without a comment. This time it was my fault.

Did you be notified about my comments? I probably didn't click reply.

Reply To lachu

Did you be notified about my comments? I probably didn't click reply.

You don't have to click reply. I get every comment. But I don't get uploaded patches.

@kvilhaugsvik : Hi. I do not known if you seen my last patch (RC3-counters.patch). I known people have job, but you (probably) are also interested in add counters implementation to Freeciv, isn't?

git am complains. Patch format detection failed. Did you create this with git show? Did you edit it by hand?

You have added an unused int i; that breaks compilation. Do you want the test patch that forces everything to compile so you can catch compile errors your self?

You added a space after Search for counter by rule name

I thought I configure build directory with --debug-enabled (or something similar). I will use this switch. Sorry.

Yes. I edit by hand, because I apply patches on my machine before publishing it and am complains about patch not apply or something similar.

I removed unused variable warning supposed to be error and whitespace problem.

I compiled with --enable-debug and compilation pass.

Please, apply only RC4-counters-initial-patch.patch (last patch on list). If you have any suggestions, please wrote.

HI Reply To kvilhaugsvik

git am complains. Patch format detection failed. Did you create this with git show? Did you edit it by hand? You have added an unused int i; that breaks compilation. Do you want the test patch that forces everything to compile so you can catch compile errors your self? You added a space after Search for counter by rule name

Hi. I apply RC4 patch on top of newest version in master. It apply, compile and working. Any suggestions?

I reread the full discussion. About id, it is not necessary. It uses a memory - I can remove it. About patches I apply RC4 to some git version without errors in past. I try to do the same today.

coding style issue: switch case alignment.

Apply RC5-Initial-commit-for-counters-Remove-id-filed-from-cou.patch

Reply To kvilhaugsvik

coding style issue: switch case alignment.

Sorry. I did not see your comment before posting previous.

I repair switch ... case ... default code.

Apply: RC6-Initial-commit-for-counters-Remove-id-filed-from-cou.patch

If you remove the default the compiler will complain when a new counter_target is added but not handled in the switch statement.

Reply To kvilhaugsvik

If you remove the default the compiler will complain when a new counter_target is added but not handled in the switch statement.

That's bad? I understood, I should handle something like COUNTER_TYPE_LAST instead of all other cases. I did not remove default from counter_target switch.

Reply To lachu

Reply To kvilhaugsvik

If you remove the default the compiler will complain when a new counter_target is added but not handled in the switch statement.

That's bad? I understood, I should handle something like COUNTER_TYPE_LAST instead of all other cases. I did not remove default from counter_target switch.

I was trying to recommend removing the default in the switch in counter_by_index() so people that add more are forced to fill it in.

Reply To kvilhaugsvik

Reply To lachu

Reply To kvilhaugsvik

If you remove the default the compiler will complain when a new counter_target is added but not handled in the switch statement.

That's bad? I understood, I should handle something like COUNTER_TYPE_LAST instead of all other cases. I did not remove default from counter_target switch.

I was trying to recommend removing the default in the switch in counter_by_index() so people that add more are forced to fill it in.

Thanks. Done.

Attach only RC8 patch. I think everything is done, sorry if not.

Reply To kvilhaugsvik

Reply To lachu

Reply To kvilhaugsvik

If you remove the default the compiler will complain when a new counter_target is added but not handled in the switch statement.

That's bad? I understood, I should handle something like COUNTER_TYPE_LAST instead of all other cases. I did not remove default from counter_target switch.

I was trying to recommend removing the default in the switch in counter_by_index() so people that add more are forced to fill it in.

Ok. No response. Please, response or find out another person, who will put these functionality to mainstream.

Reply To lachu

Ok. No response. Please, response

Sorry about that. I was at vacation and forgot my OSDN sign in.

Reply To kvilhaugsvik

Reply To lachu

Ok. No response. Please, response

Sorry about that. I was at vacation and forgot my OSDN sign in.

No problem. You also have personal live, but I response as fast as I can lately and nobody answer to my post. Because implementing this feature by me takes too long I thought you decided to not put it into freeciv.

1. When you make a change please explain what you did and why you did it

2. Why the coding style change to the definition of countersMAX_COUNTERS? (You "save a line" by moving the "{" to after the =) Is it even correct? By correct I mean: you can point to something in doc/CodingStyle, not that you personally like it better. Remember that the coding style is to make the code base consistent so it is readable by all that know the style, not what an individual developer considers pretty.

3. Why indent various stuff in conters.h that shouldn't be indented according to the coding style? A merge conflict you resolved manually and too quick? A "helpful" tool?

4. Why convert spaces to tabs before comments?

5. Why did you remove counter id? It is just an int. It is about a rule set defined counter type so there will only be one of it per counter rule.

Hint: when something is done many other places in the code base it probably follows the coding style. Don't "fix it", at least not without asking.

Reply To kvilhaugsvik

1. When you make a change please explain what you did and why you did it 2. Why the coding style change to the definition of countersMAX_COUNTERS? (You "save a line" by moving the "{" to after the =) Is it even correct? By correct I mean: you can point to something in doc/CodingStyle, not that you personally like it better. Remember that the coding style is to make the code base consistent so it is readable by all that know the style, not what an individual developer considers pretty. 3. Why indent various stuff in conters.h that shouldn't be indented according to the coding style? A merge conflict you resolved manually and too quick? A "helpful" tool? 4. Why convert spaces to tabs before comments? 5. Why did you remove counter id? It is just an int. It is about a rule set defined counter type so there will only be one of it per counter rule. Hint: when something is done many other places in the code base it probably follows the coding style. Don't "fix it", at least not without asking.

I will respond in few days. I have use some tool to automatically format C code (probably set coding style in KDevelop and reformat files).

When it comes to tools: does KDevelop have a GNU coding style like QtCreator has? In that case a copy of it is a great start to get the Freeciv coding style.

Reply To kvilhaugsvik

When it comes to tools: does KDevelop have a GNU coding style like QtCreator has? In that case a copy of it is a great start to get the Freeciv coding style.

It does.

Changes in previous patch:

- Remove id filed from counter. Id field is not necessary, because we can count index in global array by using C language pointer arithmetic - Repair style of switch ... statement - Remove default from counter_type switch (counter_by_index routine)

Id field is not necessary like somebody suggested to me previously.

Patch doesn't apply.

Do not "fix" the style of code you don't modify.

If you add a default to a enum switch clause you add you need to explain why having it is worth not getting an error message for cases not handled when someone adds a new enum value.

Reply To kvilhaugsvik

Patch doesn't apply. Do not "fix" the style of code you don't modify. If you add a default to a enum switch clause you add you need to explain why having it is worth not getting an error message for cases not handled when someone adds a new enum value.

About switch you are right. I remove assert for undefined enum values, because adding new possible value (without adding support in switch) will cause compiler alert. I hope nobody will increase/decrease or do any math on value of this enum, or (even worse) calling some function with integer in place argument of this enum type.

I use GNU formatting as somebody asking.

Reply To kvilhaugsvik

Patch doesn't apply. Do not "fix" the style of code you don't modify. If you add a default to a enum switch clause you add you need to explain why having it is worth not getting an error message for cases not handled when someone adds a new enum value.

Last patch should apply.

+/* utility */ +#include "fcintl.h"

What this is needed for? I don't see translated strings added.

+struct counter *counter_by_id (int id)

There should be no space between function name and '(' Same for all the function declarations and calls.

+ return counters_cityindex; + break;

Reply To cazfi

Sorry for asking about this. Is there still any problem with my patch. Thanks.

Reply To lachu

Reply To cazfi Sorry for asking about this. Is there still any problem with my patch. Thanks.

Sorry for not checking it. Just adding a new attachment to the ticket does not send notifications, so I were not aware that you had added a new patch version. Please add also a comment when you attach a new version.

There's still spaces between function/macro names and '(' in calls, at least those of fc_assert() family.

See CodingStyle for grouping and order of the includes; 'counters.h' should be the last one, and 'log.h' should be in '/* utility */' group with name of that group as a comment (previous version had that part right, just replace 'fcintl.h' with 'log.h')

Why you need stddef.h include? No other source file include it, so it seems surprising that this would need it. If it's for NULL, you should include it via stdlib.h instead.

Try only to fix the listed problems. I've lost count how many rounds this has now been going so that you have fixed the problems but also made other changes that have caused new problems.

I do the changes.