[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Marko Lindqvist]

Update of bug #13810 (project freeciv):

  Status:None => Duplicate  
 Assigned to:   mbook => cazfi  
 Open/Closed:Open => Closed 


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Matthias Pfafferodt]

Follow-up Comment #13, bug #13810 (project freeciv):

a new version can be found in gna 13867

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Matthias Pfafferodt]

Follow-up Comment #12, bug #13810 (project freeciv):

> Sorry if something was unclear beforehand. 
> Unfortunately I will not be able to explain so much 
> now as I will be trying to finish the work for 2.2. 

Thanks for the help! I did learn something in the
discussions and the last thing I did want is to keep
you from finishing 2.2 ;-)



___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Madeline Book]

Follow-up Comment #11, bug #13810 (project freeciv):

> This way I can use 
> 
> pset_scategory(i) 
> 
> else I would have 
> 
> pset_scategory(setting_by_number(i))

There is an example of how to create an iteration
macro when the array pointer is not available in
the header in the patch in bug #13793.

> Reading your comments I also found that I'm missing 
> some knowledge about how to code (I did learn some 
> basics about C/C++ coding but I do mainly html/php). 
> I think I have to reread the patches #13805 and 
> #13810 (and an unsend patch to add action callback 
> function) carefully with regard to variable and 
> function naming and possible simplifications. And 
> I need to do some planning before coding ...

Sorry if something was unclear beforehand.
Unfortunately I will not be able to explain so much
now as I will be trying to finish the work for 2.2.



では失礼します。

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Matthias Pfafferodt]

Follow-up Comment #10, bug #13810 (project freeciv):

> Why do you use the setting number as first argument 
> to almost every setting function? The only function 
> that takes a setting id should be setting_by_number(). 
> 
> > for (i = 0; i < SETTINGS_NUM; i++) { 

This way I can use

pset_scategory(i)

else I would have

pset_scategory(setting_by_number(i))

If the second one is prefered I will change it.

Reading your comments I also found that I'm missing
some knowledge about how to code (I did learn some
basics about C/C++ coding but I do mainly html/php).
I think I have to reread the patches #13805 and
#13810 (and an unsend patch to add action callback
function) carefully with regard to variable and
function naming and possible simplifications. And
I need to do some planning before coding ...


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Madeline Book]

Follow-up Comment #9, bug #13810 (project freeciv):

> _(pset_short_help(i))

Why not put the _() in setting_short_help() and write
in the comment header that it returns the translated
version? This goes for all such dupicated expressions.

Why do you use the setting number as first argument
to almost every setting function? The only function
that takes a setting id should be setting_by_number().

> for (i = 0; i < SETTINGS_NUM; i++) {

Use an iteration macro, either as the for loop or
using the generic iterator interface in utility/
iterator.h. Iterate on 'pset' which is a 'struct
setting *'.



なんだかむかつく。

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Madeline Book]

Follow-up Comment #8, bug #13810 (project freeciv):

I meant "pset" only for the variable names; functions
structs, etc. should have "setting". The variable name
should usually give some hint as to what it stands for.
So "cmd", when the type is "struct setting *", would
not be a good variable name (someone would think it
was a "struct command").

Also, do not do this

> pset_sval_validate(cmd)(arg, caller, &reject_message)

Just make the wrapper like

  setting_string_validate(pset, arg, ...)

and have it call the validate function if it exists,
i.e., if the function pointer is not NULL it returns
the return value of the call, otherwise it returns TRUE.



注意して読みなさい。

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Matthias Pfafferodt]

Follow-up Comment #7, bug #13810 (project freeciv):

version 4; changes:

- rename op_* to pset_* (pset = pointer to struct setting)

(file #6104)
___

Additional Item Attachment:

File name: version4-gna13810.patch.diff   Size:41 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Matthias Pfafferodt]

Follow-up Comment #6, bug #13810 (project freeciv):

version 3:

- on top of gna13805
- all changes from this ticket (gna13810) included
- summery:

* remove all dependencies on settings_s in all
  files but settings.c
* the struct definition uses an union
* rename settings_s to setting

If we trust all callers to use the right values
from the union, the asserts

assert(op->stype == SSET_BOOL);
assert(op->stype == SSET_INT);
assert(op->stype == SSET_STRING);

can be removed and settings.c can be simplified.


(file #6096)
___

Additional Item Attachment:

File name: version3-gna13810.patch.diff   Size:40 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Matthias Pfafferodt]

Follow-up Comment #5, bug #13810 (project freeciv):

new patch on top of the last one (depends on gna13805):

update union in struct settings_s + wrapper functions

* new union in struct settings_s by pepeto
* idea for wrapper functions by mbook

I also added a patch which includes both changesets.

(file #6078, file #6079)
___

Additional Item Attachment:

File name: 0001-update-union-in-struct-settings_s-wrapper-function.patch
Size:17 KB
File name: version2-gna13810.patch.diff   Size:18 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Madeline Book]

Follow-up Comment #4, bug #13810 (project freeciv):

> static inline bool setting_boolval(conct struct setting_s *op)

That should be "const" of course.

I'm also not sure about the naming scheme, since
with the full "setting_" prefix this makes it a
little awkward to use in expressions.

Perhaps just SBOOLVAL or some similar short
macro-like name (beware conflicts though, both
literal and conceptual :)).



カンック？

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Madeline Book]

Follow-up Comment #3, bug #13810 (project freeciv):

> It looks really better and understandable to me. More
> over, it should take the minimal size of the whole
> structure, and not an addition of all of them.

I agree with pepeto, the "union of structs" would
probably make more sense, since what we are basically
trying to do is polymorphism on the structs in
pepeto's example code.



もうちょっとお願いがあるんですが。

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Madeline Book]

Follow-up Comment #2, bug #13810 (project freeciv):

The "*(op->value.bval)" and similar expressions would
make good candidates for wrapper macros, so that the
internal field names can be easily updated in the
future if needed.

Actually, probably static inline wrapper functions
would be best, e.g.:

/* In settings.h */
static inline bool setting_boolval(conct struct setting_s *op)
{
  return *(op->value.bval);
}
/* And similarly for int, str. */

I would suggest to call the pointer union something
other than "value", since it is not the value itself
(recall you were confused by this a while ago). Perhaps
"pvalue" or "valaddr" would be more fitting.



全力を尽くして十分に成功する。

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Madeline Book]

Update of bug #13810 (project freeciv):

 Assigned to:None => mbook  


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[pepeto]

Follow-up Comment #1, bug #13810 (project freeciv):

Why not using the union like:
union {
  struct {
bool *value;
const bool default_value;
const bool_validate_func_t validate;
  } boolean;
  struct {
int *value;
const int default_value;
const int min_value;
const int max_value;
const int_validate_func_t validate;
  } integer;
  struct {
char *value;
const size_t value_size;
const char *default_value;
const string_validate_func_t validate;
  } string;
};

It looks really better and understandable to me. More over, it should take
the minimal size of the whole structure, and not an addition of all of them.


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #13810] [patch] use unions to save the game settings

[Matthias Pfafferodt]

URL:
  

 Summary: [patch] use unions to save the game settings
 Project: Freeciv
Submitted by: syntron
Submitted on: Sonntag 28.06.2009 um 22:07
Category: general
Severity: 2 - Minor
Priority: 5 - Normal
  Status: None
 Assigned to: None
Originator Email: 
 Open/Closed: Open
 Discussion Lock: Any
 Release: 
Operating System: None

___

Details:

see $summery

this patch is on top of gna13805

gna13807 can be closed as this is a better solution




___

File Attachments:


---
Date: Sonntag 28.06.2009 um 22:07  Name:
0001-use-unions-to-save-the-game-settings.patch  Size: 16kB   By: syntron



___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev