[Freeciv-Dev] Re: Inconsistent function names and arguments

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Jul 28, 2000 at 02:45:57PM +1000, David Pfitzner wrote:
> Jeff Mallatt wrote:
> > At 2000/07/24 09:37 , Raimar Falke wrote:
> > >On Mon, Jul 24, 2000 at 06:56:44AM -0400, Jeff Mallatt wrote:
> 
> > >> /* same as current tech_flag() */
> > >> int tech_type_has_flag(Tech_Type_id tech_type, enum tech_flag_id flag);
> 
> > >> /* same as current unit_flag() */
> > >> int unit_type_has_flag(Unit_Type_id unit_type, enum unit_flag_id flag);
> > >> 
> > >> /* this takes a pointer to a unit, not to a unit type */
> > >> int unit_has_flag(struct unit *punit, enum unit_flag_id flag);
> 
> > >This were the changes I had in mind. I thing is IMHO missing: the 
> > >difference
> > >between poiners and ids:
> > >
> > >int unit_has_flag(Unit_Type_id unit_id, enum unit_flag_id flag);
> > >int punit_has_flag(struct unit *punit, enum unit_flag_id flag);
> > 
> > This difference is the difference between ????_has_flag and
> > ????_type_has_flag in my proposal.  It's just subjective, but I don't like
> > the name punit_has_flag.
> 
> I agree about the name "punit_has_flag", in that currently function 
> names don't use the form "punit" (or p-anything), only variables use 
> that.

I'm sorry for late replying.

The p-thing is only a suggestion. I thought it would be easy to understand. 
This last statement is not correct. There are functions:
  government.h:struct government *get_gov_pcity(struct city *pcity);
  government.h:struct government *get_gov_pplayer(struct player *pplayer);
  player.h:const struct player_diplstate *pplayer_get_diplstate(const struct 
player *pplayer,
  player.h:int pplayers_at_war(const struct player *pplayer,
  player.h:int pplayers_allied(const struct player *pplayer,
  player.h:int pplayers_non_attack(const struct player *pplayer,

> But I'm guessing maybe there is confusion above between unit types 
> (type Unit_Type_id, valid range 0 to <U_LAST) and unit ids (currently 
> type int, range 1 to about INT_MAX (?)), and Raimar's example 
> should perhaps more clearly be the 3 functions:
> 
>   int unit_type_has_flag(Unit_Type_id unit_type, enum unit_flag_id flag);
>   int punit_has_flag(struct unit *punit, enum unit_flag_id flag);
>   int unit_has_flag(int unit_id, enum unit_flag_id flag);

And to be complete:
    int punit_type_has_flag(struct unit_type *punit_type, 
                            enum unit_flag_id flag);

> But if so I don't much like the idea of having a unit_id version -- 
> better for the caller to call find_unit_by_id() and then use the result 
> (which encourages the caller to do this once, and cache the result). 

I agree.

> And if we were to have a unit_id version, for better consistency with 
> the bulk of existing code, I would prefer the names be, eg:
> 
>   int unit_type_has_flag(Unit_Type_id unit_type, enum unit_flag_id flag);
>   int unit_has_flag(struct unit *punit, enum unit_flag_id flag);
>   int id_unit_has_flag(int unit_id, enum unit_flag_id flag);

    int id_unit_type_has_flag(Unit_Type_id unit_type, 
                              enum unit_flag_id flag);
    int unit_type_has_flag(struct unit_type *punit_type,
                           enum unit_flag_id flag);

If we can agree on this style I will start to produce some patches.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "We just typed make..."
    -- Stephen Lambrigh, Director of Server Product Marketing at Informix,
                         about porting their Database to Linux