05 Jan, 2009, Tricky wrote in the 1st comment:
Votes: 0
When compiling the latest (rev 67) Ice source for RaM under a Cygwin environment, I got the following error.
gcc -Werror -Wall -Wformat -Wformat-security -Wmissing-format-attribute
-Wmissing-braces -Wparentheses -Wshadow -Wredundant-decls -Wcast-qual
-Wcast-align -Wchar-subscripts -Wreturn-type -Wswitch -Wwrite-strings
-Wunused -Wuninitialized -Wpointer-arith -Winline -Wmissing-declarations
-Wmissing-prototypes -Wstrict-prototypes -O3 -g -c comm.c -o comm.o
In file included from comm.c:71:
merc.h:2009: warning: declaration of `stat' shadows a global declaration
/usr/include/sys/stat.h:124: warning: shadowed declaration is here
merc.h:2010: warning: declaration of `stat' shadows a global declaration
/usr/include/sys/stat.h:124: warning: shadowed declaration is here
make: *** [comm.o] Error 1

I solved this with the following patch…
diff -ur ./Ice/src/Makefile ./Ice/src/Makefile
— ./Ice/src/Makefile 2009-01-05 02:51:58.000000000 +0000
+++ ./Ice/src/Makefile 2009-01-05 03:07:05.136248000 +0000
@@ -51,6 +51,10 @@
save.o sha256.o skills.o special.o strings.o \
tables.o update.o

+default: all
+
+all: dep ram
+
ram: $(O_FILES)
@rm -f $@
$(CC) $(L_FLAGS) -o $@ $(O_FILES) $(LIBS)
diff -ur ./Ice/src/handler.c ./Ice/src/handler.c
— ./Ice/src/handler.c 2009-01-05 02:51:58.000000000 +0000
+++ ./Ice/src/handler.c 2009-01-05 03:07:05.156276800 +0000
@@ -866,7 +866,7 @@
}

/* command for retrieving stats */
-int get_curr_stat( const CHAR_DATA *ch, int stat )
+int get_curr_stat( const CHAR_DATA *ch, int _stat )
{
int max = 0;

@@ -875,9 +875,9 @@

else
{
- max = pc_race_table[ch->race].max_stats[stat] + 4;
+ max = pc_race_table[ch->race].max_stats[_stat] + 4;

- if ( class_table[ch->iclass].attr_prime == stat )
+ if ( class_table[ch->iclass].attr_prime == _stat )
max += 2;

if ( ch->race == race_lookup( "human" ) )
@@ -886,19 +886,19 @@
max = UMIN( max, 25 );
}

- return URANGE( 3, ch->perm_stat[stat] + ch->mod_stat[stat], max );
+ return URANGE( 3, ch->perm_stat[_stat] + ch->mod_stat[_stat], max );
}

/* command for returning max training score */
-int get_max_train( CHAR_DATA *ch, int stat )
+int get_max_train( CHAR_DATA *ch, int _stat )
{
int max = 0;

if ( IS_NPC( ch ) || ch->level > LEVEL_IMMORTAL )
return 25;

- max = pc_race_table[ch->race].max_stats[stat];
- if ( class_table[ch->iclass].attr_prime == stat )
+ max = pc_race_table[ch->race].max_stats[_stat];
+ if ( class_table[ch->iclass].attr_prime == _stat )
{
if ( ch->race == race_lookup( "human" ) )
max += 3;
diff -ur ./Ice/src/merc.h ./Ice/src/merc.h
— ./Ice/src/merc.h 2009-01-05 02:51:58.000000000 +0000
+++ ./Ice/src/merc.h 2009-01-05 03:07:05.176305600 +0000
@@ -2129,8 +2129,8 @@
void reset_char( CHAR_DATA *ch );
int get_trust( const CHAR_DATA *ch );
int get_age( CHAR_DATA *ch );
-int get_curr_stat( const CHAR_DATA *ch, int stat );
-int get_max_train( CHAR_DATA *ch, int stat );
+int get_curr_stat( const CHAR_DATA *ch, int _stat );
+int get_max_train( CHAR_DATA *ch, int _stat );
int can_carry_n( CHAR_DATA *ch );
int can_carry_w( CHAR_DATA *ch );
bool is_name( const char *str, const char *namelist );

The Makefile patch was just to make sure dep was automatically made.

Obviously using _stat isn't ideal, however I am sure you can find a proper variable name for it.

Tricky

Edit: Forgot to mention it works/runs with a few unknown skills warnings in the log file. Also changed rom to ram in the startup script in ./Ice/area
05 Jan, 2009, quixadhal wrote in the 2nd comment:
Votes: 0
Thanks!

Yeah, I'm kindof curious where cygwin includes stat.h, as linux apparently doesn't put it in the same place.
The makefile patch will probably make things cleaner. I hadn't put in a rule since the error isn't fatal under GNU make, but I did dislike seeing the error message.
05 Jan, 2009, Tricky wrote in the 3rd comment:
Votes: 0
Forgot to mention that gcc is only 3.4.4 atm under Cygwin.

fcntl.h -> sys/fcntl.h -> sys/stat.h

You still see the error message on a clean compile, however putting a '-' before the include statement masks that. I forgot to put it in.

-include dep


Tricky
05 Jan, 2009, Tyche wrote in the 4th comment:
Votes: 0
The error is caused by the "-Werror -Wshadow" combination. Because it's not an error at all, as it's perfectly legal and proper C to declare local variables that shadow globals. Also -Wparentheses cause perfectly legal and intelligible C to be flagged as errors.
05 Jan, 2009, Tyche wrote in the 5th comment:
Votes: 0
quixadhal said:
Yeah, I'm kindof curious where cygwin includes stat.h, as linux apparently doesn't put it in the same place.


It's a newlib v. glibc difference. You might encounter endless variances in the global namespace on many different unices and versions.
So -Wshadow is platform and even version dependent.
05 Jan, 2009, quixadhal wrote in the 6th comment:
Votes: 0
I do know it's perfectly legal to shadow definitions in local namespaces, but I've found that the vast majority of the time, it's not done because of someone trying to reuse code, but rather someone doing a cut-and-paste job, or not realizing that they just covered up something global.

In big projects that pass through dozens of hands over the years, it usually causes more harm than good, as someone sees a global variable "buf", and 12 pages down does a strcpy into it and then 7 pages further down, can't figure out why buf has the "old" value. That's actually the main (and really only) reason I don't like declaring variables inside scope brackets vs. the old school way of having to put them at the top of the function.

Using -Werror is certainly something that has caused much debate, but I stand by it, since it's a very rare construct that can't be worked around, and it forces people to deal with little errors while they're still little, rather than ignoring the warnings until there are so many they don't have the fortitude to sit down and tackle them all.

And, if somebody does just need their code to compile, and really intends to deal with the warnings later (or give me the finger and ignore them forever!), it's easy to edit that flag out of the Makefile. :)
05 Jan, 2009, Tyche wrote in the 7th comment:
Votes: 0
No -Werror is fine. The issue is -Wshadow and -Wparentheses flag perfectly good C code. The problem is the patch likely just reintroduces the same warning (in this case error) into those trying to compile with MinGW or DevC++ as _stat is probably defined by their LIBC headers (haven't tested it). In the case of -Wshadow you insist that the local names never conflict with global names. You make the code unportable and subject to the whimsy of global namespace pollution of different versions of gcc, libc, third party libraries and different unix flavors. They are advisory warnings.

quixadhal said:
I do know it's perfectly legal to shadow definitions in local namespaces, but I've found that the vast majority of the time, it's not done because of someone trying to reuse code, but rather someone doing a cut-and-paste job, or not realizing that they just covered up something global.


Well this case in point, declaring a local variable called 'stat' is bad?

Let me give you contrived examples..
suppose I wanted to include a third party library into my project. And it's header declares functions named i(), in(), name(), out(), etc. Then I'd have to change every local variables by the same name in all my code. That's why I think it's a silly sort of warning.
05 Jan, 2009, Tricky wrote in the 8th comment:
Votes: 0
This is why I mentioned that the variable name _stat was not ideal in my post.

Tricky
05 Jan, 2009, quixadhal wrote in the 9th comment:
Votes: 0
IMO, it's bad because it shares the same name as the stat() function call. It also looks ugly to see things like ch->mod_stat[stat] = 0. I renamed it to stat_index, since that's how it's used.

Then again, I was perfectly happy with dprintf(), back in 1995. Now, it's a glibc call, so I had to rename it to desc_printf()… that being a function to print to descriptors.
05 Jan, 2009, Tyche wrote in the 10th comment:
Votes: 0
Tricky said:
This is why I mentioned that the variable name _stat was not ideal in my post.

Tricky


Any variable name the function itself does not otherwise use ought to be fine.
If you want to declare is as int arctan that ought to be good.
It's -Wshadow that's stupid, IMHO. ;-)
05 Jan, 2009, Tyche wrote in the 11th comment:
Votes: 0
quixadhal said:
Then again, I was perfectly happy with dprintf(), back in 1995. Now, it's a glibc call, so I had to rename it to desc_printf()… that being a function to print to descriptors.


Those errors aren't flagged by -Wshadow. -Wshadow flags local variables as invalid. C isn't COBOL. It's got scope.
0.0/11