[Freeciv-Dev] Re: [RFC] enum cleanup

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Feb 13, 2002 at 02:42:20PM +0000, Ben Webb wrote:
> On Wed, 13 Feb 2002, Raimar Falke wrote:
> 
> > ./common/improvement.c:302:  for (i=0;i<EFR_LAST;i++) implist[i]=NULL;
> > ./common/improvement.c:338:    for (i=0;i<EFR_LAST;i++) {
> > ./common/improvement.c:350:      for (i=0;i<EFR_LAST;i++) {
> > ./common/improvement.c:547:  for (i=0; i<EFR_LAST; i++)
> > ./common/improvement.c:560:  for (j=0; j<EFR_LAST; j++) {
> > 
> > To be type safe this has to be replaced with:
> >   for(i = X_FIRST; i < X_LAST; i++)
> 
>       The only problem with this is that there isn't an EFR_FIRST 
> defined, rather EFR_NONE. This of course then creates problems if the 
> order of EFR_XXX is changed such that EFR_NONE is no longer first.
> 
> What about
> typedef enum {
>   EFR_FIRST,  /* keep this first */
>   EFR_NONE = EFR_FIRST,
>   EFR_BUILDING,
>   EFR_CITY,
>   EFR_ISLAND,
>   EFR_PLAYER,
>   EFR_WORLD,
>   EFR_LAST      /* keep this last */
> } Eff_Range_id;
> 
> ? Ugly, but it should avoid that problem.

government.h has:

enum government_flag_id {
  G_BUILD_VETERAN_DIPLOMAT=0,   /* and Spies (in general: all F_DIPLOMAT) */
...
  G_LAST_FLAG
};
#define G_FIRST_FLAG G_BUILD_VETERAN_DIPLOMAT

So *_FIRST is a define. I would prefer this.

> (On the other hand, EFR_LAST isn't really the "last" element - it's
> the one after the last. But I guess EFR_ONEAFTERLAST would look even
> more ugly.)

Yes I also noticed this.

> > And last but not least: there is
> >   typedef int Impr_Type_id;
> > which is used in the code. And there is
> > enum improvement_type_id {
> >   B_AIRPORT=0, B_AQUEDUCT, B_BANK, B_BARRACKS, B_BARRACKS2, B_BARRACKS3,
> >   B_CATHEDRAL, B_CITY, B_COASTAL, B_COLOSSEUM, B_COURTHOUSE,  B_FACTORY,
> >   B_GRANARY, B_HARBOUR, B_HYDRO, B_LIBRARY, B_MARKETPLACE, B_MASS, B_MFG,
> >   B_NUCLEAR, B_OFFSHORE, B_PALACE, B_POLICE, B_PORT, B_POWER,
> >   B_RECYCLING, B_RESEARCH, B_SAM, B_SDI, B_SEWER, B_SOLAR, B_SCOMP,
> >   B_SMODULE, B_SSTRUCTURAL, B_STOCK, B_SUPERHIGHWAYS, B_SUPERMARKET, 
> > B_TEMPLE,
> >   B_UNIVERSITY,
> > 
> >   B_APOLLO, B_ASMITHS, B_COLLOSSUS, B_COPERNICUS, B_CURE, B_DARWIN, 
> > B_EIFFEL,
> >   B_GREAT, B_WALL, B_HANGING, B_HOOVER, B_ISAAC, B_BACH, B_RICHARDS,
> >   B_LEONARDO, B_LIGHTHOUSE, B_MAGELLAN, B_MANHATTEN, B_MARCO, 
> > B_MICHELANGELO,
> >   B_ORACLE, B_PYRAMIDS, B_SETI, B_SHAKESPEARE, B_LIBERTY, B_SUNTZU,
> >   B_UNITED, B_WOMENS,
> >   B_CAPITAL, B_LAST_ENUM
> > };
> > 
> > These two should be merged into
> > 
> > typedef enum {...} Impr_Type_id.
> 
>       I disagree. The enum improvement_type_id will (hopefully) be 
> completely redundant in the near future, and so will be able to be removed 
> entirely, so we end up with just "typedef int Impr_Type_id;" Rolling the 
> enum into the Impr_Type_id just makes impr_gen harder in the long term.

I don't see the problem. The code will now and later use
"Impr_Type_id". The code uses now B_* which should be an element of
Impr_Type_id.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "This is Linux Country. On a quiet night, you can hear Windows reboot."