[Freeciv-Dev] (PR#9748) (PR #4712) Comments about version.diff

["Marko Lindqvist" <marko.lindqvist@xxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9748 >

  This patch is almost year old and touch a lot of places, so I'm not 
even trying to apply it, just reading through. I think that something 
like this would be quite useful cleanup under C -parts of my versioning 
patches, even when this means rewriting for me. This does not affect 
first part of my patches, one_versionfile_config.diff.


  - In several places, same function is called twice in a row 
determining parameters for another function call. Even if called 
function is quite trivial, and those calls are not in speed critical 
parts of code, this just looks ugly to my eyes:
DrawText(hdc, version_string(), strlen(version_string()), &rc, DT_CENTER);
  Second time around, I noticed that most of those were not actually 
added by this patch, but they were just close enough to touched parts to 
be included in diff.
  But there is not too many ways around this, I'm afraid. Functions 
could be made something like following, but it would be ugly too.

const char *version_string(int *return_length) {
   my_snprintf(version, sizeof(version), "%d.%d.%d%s", MAJOR_VERSION,
               MINOR_VERSION, PATCH_VERSION, VERSION_LABEL);
   if (NULL != return_length) {
     *return_length = strlen(version);
   }
   return version;
}

  - Internally version.c still uses those macros. They should be used 
only in their respective get functions. Only reason to keep them as 
macros at all is to make their values appear on top of file, easily 
editable.

  - NEXT_RELEASE_MONTH should be just an integer, for easier editing. 
You could get rid of that null -element at index 0 of month array too ( 
month[NEXT_RELEASE_MONTH-1] )

  - Comment inside version.c should not refer to version.c as if it was 
different file: "The month[] array is defined in version.c"

  - config.h changes in diff prove that patch compilation is tested with 
different configs?

  - I think that save_version() should be in savegame.c, as its logic is 
savegame related. It gives different results for different versions, but 
thats only because it (should) call major_version(), minor_version() and 
patch_version() from version.c

  - Moving save_game_auto() -call in srv_main() seems unrelated to this 
patch. Included in error?

  - Some config.h changes are included in diff. 1) Comments generated in 
config.h prove that there are changes in configure.in/ac but no such 
changes are in diff. 2) Comments are wrong claiming that config.h 
version information is deprecated; it's still primary source of those 
macros while version.c provides only fall backs.


  - Caz