08 Oct, 2008, MacGregor wrote in the 81st comment:
Votes: 0
quixadhal said:
MacGregor said:
They are -Wuninitialized, -Wmissing-prototypes, -Wstrict-prototypes and -Wmissing-declarations. And, uhh, do we really need -Winline? Do we *care* if the compiler is unable to inline a function?


I added those to the last patch, although a few of them are only allowed for the C language, and that actually varies by compiler version!

I don't think I've ever seen the compiler cough on -Winline, I added that years ago when I was working on a low-CPU power machine and actually tried to inline lots of tiny functions. I don't think it matters, although if it doesn't break anything I'd be tempted to leave it there anyways. The keyword inline doesn't actually appear in any of the code to date.

Okay, that part about variation from compiler version to compiler version concerns me. Do you mean the warnings have different meanings on different versions, or just that not all versions support all warnings?

As for the -Winline, I'd say let's get rid of it. It's not going to do anything for us, and it's just cluttering up an already busy Makefile.

Also, I wonder how much these warnings even work. Consider the ones that I was coping with, the ones caused by -Wcast-qual. According to my documentation, that means "Warn whenever a pointer is cast so as to remove a type qualifier from the target type. For example, warn if a `const char *' is cast to an ordinary `char *'." And that's exactly what statements like "OBJ_DATA *obj1 = (OBJ_DATA *) arg1;" are doing; if gcc 4.1.2 didn't complain about that then the warning is broken in 4.1.2. And no, I don't believe gcc is smart enough to figure out that the variable doesn't really get changed, and I suspect that in your heart of hearts, neither do you.
08 Oct, 2008, quixadhal wrote in the 82nd comment:
Votes: 0
MacGregor said:
Okay, that part about variation from compiler version to compiler version concerns me. Do you mean the warnings have different meanings on different versions, or just that not all versions support all warnings?

A bit of both. They have the same meanings, but they may not be implemented for all languages, for all versions of the compiler suite.

As you move forward, more of them are available. Since I (and the Debian people) consider the 4.1 series to be "stable", that's what I consider the most important version to use. As of my last set of compiles, there was one warning which wasn't available prior to the 3.x series, so I added a comment to the Makefile telling people to upgrade or disable that flag. There was also one flag which was availalbe in 4.3, but not in 4.1, so I disabled that one with a comment telling people who are using the newer compiler to enable it.

Welcome to the wonderful world of free software. :)

MacGregor said:
As for the -Winline, I'd say let's get rid of it. It's not going to do anything for us, and it's just cluttering up an already busy Makefile.

That's fine. It's only one flag. We can make the compile line silent if you think it'll scare away the customers.
This makefile is very simple compared to some… I resisted allowing it to become sentient (self-modifying) for the dependancy generation. I do that for mine, but I'd hate for someone to hurt themselves by having it eat their makefile and not knowing how to get it back.

MacGregor said:
Also, I wonder how much these warnings even work. Consider the ones that I was coping with, the ones caused by -Wcast-qual. According to my documentation, that means "Warn whenever a pointer is cast so as to remove a type qualifier from the target type. For example, warn if a `const char *' is cast to an ordinary `char *'." And that's exactly what statements like "OBJ_DATA *obj1 = (OBJ_DATA *) arg1;" are doing; if gcc 4.1.2 didn't complain about that then the warning is broken in 4.1.2. And no, I don't believe gcc is smart enough to figure out that the variable doesn't really get changed, and I suspect that in your heart of hearts, neither do you.


LOL! Actually, I can very easily see how the compiler could check that. When you declare a variable to be constant, you're saying that the memory backing that variable is to be considered read-only. During the course of generating the assembly code, the compiler can easily catch any "normal" attempts to write to any memory that was declared const. Of course, there are ways around it, but 99% of the code won't deliberately try to hide a write.

BTW, a const char * is NOT a constant pointer. It's a pointer TO a constant string (or array of charcters). I made use of that in the last patch, because it means you can declare a const char * and move that pointer around. Specifically, you'll see there are places I copied a const string into a local char * buffer, and then pointed a const char * pointer at it so I could hand THAT off to functions that require a constant string, but still modify the buffer. *shakes fist at smash_tilde*
09 Oct, 2008, quixadhal wrote in the 83rd comment:
Votes: 0
Just an FYI…. new patch is up. This one removes all the antique OS cruft. If any non-linux people have any problems, please let me know. I'm mostly hoping FreeBSD/OS X will be happy, anything else is probably too small an audience. I would expect Cygwin to behave like a mixture of BSD/SySV somewhere.
09 Oct, 2008, MacGregor wrote in the 84th comment:
Votes: 0
I had a problem with 007_antique_OS.diff:

4$ patch -p1 <../patches/007_antique_OS.diff 
patching file HACKLOG
patching file src/Makefile
Hunk #1 FAILED at 1.
1 out of 3 hunks FAILED – saving rejects to file src/Makefile.rej


No problems with the first six patches.
10 Oct, 2008, quixadhal wrote in the 85th comment:
Votes: 0
Hmmm, it worked ok from my end. Maybe it's a change in tabs or spaces or something. The changes I made were adding the ID tag, sorting the object files list and making them 5 entries per line, replacing the hard coded targets with the $@ symbol, and making a new OPTIONS variable to hold everything except the warning flags, so they can be passed to the dependancy generation line.

Part of that is the 4 defines for OLD_RAND and friends… which are the only define-based options in the code. I almost stripped out that one, because I don't think random number generators are rocket science these days, but I figured I'd ask if anyone cared first.

This brings up a question…. if we re-upload a file with the same filename, will it overwrite the old one? Or do we have to live with our mistakes unless we can bug an admin to delete them? I ask, because I had considered re-releasing the diffs directly from svn, just to be consistent.
10 Oct, 2008, Davion wrote in the 86th comment:
Votes: 0
if you look at your file, near the dowload link is one called update. This will over write the file and tag it as updated.
10 Oct, 2008, quixadhal wrote in the 87th comment:
Votes: 0
Thanks Davion!

When I'm done with my next batch of stuff, I'll re-download all the patches and replace anything that won't apply cleanly. I suspect something got changed slightly after one of the patches escaped, and so the next one was a bit off.
11 Oct, 2008, quixadhal wrote in the 88th comment:
Votes: 0
Since this seems to be the general announcement thread (good name for it too!), I just wanted to make a quick couple of notes.

I've re-uploaded all the patch files, so this should get around the couple of rejects that managed to sneak into them. I've also re-uploaded the tarball, just to be sure I didn't miss anything. I converted my local directory to SVN, and probably had a minor snafu in that process, so these are all diffs against exports of the appropriate revisions. If you're curious, that's where the numbers in the filenames come from.

I'm also happy to report that Davion has gotten us a public SVN repository (/cheer!), and it's now also loaded up to that same version of the code. I'll let him post the URL when he's sure everything is ship-shape. :)

I have my next revision ready to go. It attempts to clean up some of the mess that is merc.h and all those args() style prototypes all over the place. I haven't checked it in to the official repository yet, as I wanted to get a bit of feedback from ya'll about just how "sacrosanct" these kinds of changes are. I'll upload a patch for you to peruse though!

Here's the entry from my HACKLOG for this new version, let me know if anything sounds fishy.

081010  Quixadhal
0928 Working on removing the args() prototyping and replacing it
with something a bit simpler. In the process, I'm also trying
to clean up the header files and do a very tiny amount of
refactoring to move similar purpose things into similar
places.

TODO - Fix debug malloc support. Apparently this is from a
very old version of dmalloc.

1533 I think a serious refactoring of the file layout is long
overdue. The string handling code seems to be scattered
between recycle.c and db.c. wiznet is in the act_wiz.c
file, yet really it belongs with the rest of the channels
and send_to_char style functions.

I'm at a point where everything compiles, so I will do a
check-in and diff here. Right now, the stuff I've moved
around is pretty minor.

Here's a list of changes:

1 Removed telnet.h – use the system headers, Luke!
2 Removed extern function prototypes from all the source
files.
3 Added function prototypes for every(!) function in the
header files.
4 Created act.h for things related to any of the act_*.c
files.
5 Created special.h to hold the spec_proc stuff.
6 Merged db2.c into db.c
7 Merged magic2.c into magic.c
8 Added new include dependancies, since some things are
now in db.h, act.h, or elsewhere that isn't merc.h
9 As a result of 2-5, the DECLARE_foo() macros aren't
actually used, although they're still there. I don't
like them (it's easier to cut and paste declarations,
and then you aren't hiding how they work).
10 In the process of doing all the above, removed some
of the inconsistent blank lines.. IE: 3 lines between
foo() and foo2(), but 2 lines between foo2() and foo3().
11 Oct, 2008, MacGregor wrote in the 89th comment:
Votes: 0
Some of this sounds good to me, some of it I wish to comment on.
Items 1, 2, 6 and 7 are definitely good ideas (I have been telling myself for years I really should merge db.c with db2.c in my mud and have yet to get around to it).

I'm assuming that #3 primarily refers to the local function definitions at the tops of many (most?) source files. If they are indeed local they should be declared static; if they're called from another module they should be placed in a header file. In any case every function should have a prototype; that's what the -Wmissing-prototypes flag enforces.

I'm guessing from 4 and 5 that you're thinking of moving prototypes out of merc.h altogether? This wouldn't be a bad thing; if srcfile.c has foo() and bar() in it and they're called from other modules, then they should have prototypes in srcfile.h.

On #9, the DECLARE_xxx_FUN macros were never strictly necessary; just a bit of a convenience. I kind of like them but can certainly live without them. One thing they do accomplish, however, is document the purpose of the functions they're used to declare; DECLARE_DO_FUN( do_look ) tells you at a glance that do_look implements a mud command, DECLARE_SPELL_FUN( spell_sleep ) implements a spell, etc., although admittedly all spells start with spell_, all spec procs with spec_, etc. We could tweak their definitions to be a bit more useful a la the way Circle uses (used?) ACMD, but perhaps a bit more readable; I think somebody has already suggested something along these lines.

RoM has global variables scattered all throughout the source. They're mostly in db.c and some in comm.c but they're really likely to be anyplace. I collected them all into a single file, globals.c, in my mud. That way I didn't have them cluttering up the tops of db.c etc. Variables of file scope of course stay where they are, and get declared static. Yes we shouldn't have globals according to the CS profs, but please, let's save that discussion for another time and another place.

Anyway, looking good so far! When/if I get svn access I can pitch in on some of my suggestions here.
12 Oct, 2008, Davion wrote in the 90th comment:
Votes: 0
What should be good about 32bit vs 64bit compilers? Our current version doesn't compile under a 64 bit compiler. I figured there'd be a cross-platform format character for sizeof, but it appears not to be the case.
12 Oct, 2008, MacGregor wrote in the 91st comment:
Votes: 0
Ack, that sounds bad – yeah, we need to compile with 64-bit compilers; someday they'll be the only game in town (except for the new 128-bit systems).

What sort of error(s) are you getting?

On strings, continuing from my last post, yes, I think we should collect all the string utilities into their own file (I believe there are a few in handler.c also). The logical name to give it would be strings.c, but when we add OLC we will be adding a file called string.c We could rename string.c (to crappy_editor.c maybe?) at the cost of confusing future RoM hackers expecting strings.c to be something completely different. str_utils.c maybe?
12 Oct, 2008, Davion wrote in the 92nd comment:
Votes: 0
db.c: In function char* str_dup(const char*):
db.c:2681: error: cast from const char* to unsigned int loses precision
cc1plus: warnings being treated as errors
db.c: In function void do_dump(CHAR_DATA*, const char*):
db.c:2816: warning: format %8d expects type int, but argument 4 has type long unsigned int
db.c:2816: warning: format %d expects type int, but argument 6 has type long unsigned int
db.c:2824: warning: format %8d expects type int, but argument 4 has type long unsigned int
db.c:2824: warning: format %d expects type int, but argument 6 has type long unsigned int
db.c:2834: warning: format %8d expects type int, but argument 4 has type long unsigned int
db.c:2834: warning: format %d expects type int, but argument 6 has type long unsigned int
db.c:2846: warning: format %8d expects type int, but argument 4 has type long unsigned int
db.c:2861: warning: format %8d expects type int, but argument 4 has type long unsigned int
db.c:2861: warning: format %d expects type int, but argument 6 has type long unsigned int
db.c:2869: warning: format %8d expects type int, but argument 4 has type long unsigned int
db.c:2869: warning: format %d expects type int, but argument 6 has type long unsigned int
db.c:2873: warning: format %8d expects type int, but argument 4 has type long unsigned int
db.c:2877: warning: format %8d expects type int, but argument 4 has type long unsigned int


The issue is with 'sizeof' being in the argument list for a printf statement. If I use %ld it works fine, but I'm unsure of how that looks on a 32 bit system. I'll let you know soon.

Ya, changing it to %ld fails to compile on 32bit systems.
12 Oct, 2008, MacGregor wrote in the 93rd comment:
Votes: 0
Ouch. Well, for what it's worth, sizeof is used seldom enough in a printf type function that it won't be too bad tracking them down and casting to ints. Or is gcc gonna bitch about "loss of precision" there, too?

On a side note, do they really think a struct/class/union might be so big that the number representing its size won't fit in 32 bits?
12 Oct, 2008, Kline wrote in the 94th comment:
Votes: 0
I use %ld for a number of things on a 32 bit system with no issues…There are also ways to compile 32bit on a 64bit compiler that just involve some gcc flags and libs. Try adding -m32 to your CC line.
12 Oct, 2008, David Haley wrote in the 95th comment:
Votes: 0
There's a printf specifier that is "number of pointer size", but unfortunately it's not standard so gcc warns about it too… In my experience the best solution is to use #ifdefs to see if you're running in 64 bit, and use the appropriate specifier. 64-bit compilation is a must, IMO. Besides, well written code shouldn't really care about 32 vs. 64 bits in the first place. (Except for weirdness like this.)
12 Oct, 2008, quixadhal wrote in the 96th comment:
Votes: 0
Davion said:
db.c:2816: warning: format %8d expects type int, but argument 4 has type long unsigned int

The issue is with 'sizeof' being in the argument list for a printf statement. If I use %ld it works fine, but I'm unsure of how that looks on a 32 bit system.

To be proper, you need to use %lu to get the unsigned part as well. I would guess it's complaining because on a 64-bit platform, sizeof() will return a 64-bit value (since, in theory, your structure can be > 4G in size). That should work fine on a 32-bit platform as well, since sizeof() would be 32-bit there, as will the long type.

As far as standards go… no, there are none. FWIW, on my old Amiga Lattice C compiler, an "int" was 16-bit, and a "long" was 32-bit. Nowadays, an "int" and a "long" tend to both be either 32-bit or 64-bit, depending on the platform.

Trying to stamp down a type that you know will be 32-bits on every platform is something that's not easy to do, as a search on google will show. Each compiler will often have built-in defines which THEY enforce, but I don't believe it's part of the (ANSI) standard.
12 Oct, 2008, quixadhal wrote in the 97th comment:
Votes: 0
I've been poking around on the net for a bit now, and can't find a VM solution that will let me run an emulated-CPU out of the box… so it looks like Davion is our volunteer 64-bit tester. :biggrin:

One thing I can say is that it's probably worth doing a pass over the whole codebase to determine what places really NEED to use integers other than "int", and what places can be changed to just "int".

Since "int" is most often what people actually want when dealing with things that aren't really supposed to be either pointers or characters, I'd be inclined to replace all the "shorts" and "longs" unless there's a good reason not to. The only places it should matter are places where pointers get involved (IE: pointer arithmetic) and evil things that were done when we ran these on the C64, like funky bitmasks that assumed 7-bits or some such.

Maybe I'll scrounge up enough pocket lint to get a 64-bit barebones system for Christmas or something… or win the lotto and buy one for all the project developers. Might as well dream, eh?
12 Oct, 2008, quixadhal wrote in the 98th comment:
Votes: 0
Crap. Sorry guys, I forgot to double-check with gcc instead of g++ and apparently gcc finds more in the way of redundant declarations. I'll have another patch out the door soon to address this.
12 Oct, 2008, quixadhal wrote in the 99th comment:
Votes: 0
It is probably worth noting that gcc 2.95 is no longer available on Debian testing, and that g++ 3.4 is also no longer available (although gcc is). I thought you might find it interesting to see a comparison of file sizes where the only change was which compiler was used. Note that using g++ adds a flat overhead, but moving from version 4.1 to 4.3 reduces the size, whereas going from 3.4 to 4.1 increases the size.

-rwxr-x— 1 quixadhal quixadhal 1023978 Oct 12 02:27 rom++-41
-rwxr-x— 1 quixadhal quixadhal 1013666 Oct 12 02:28 rom++-42
-rwxr-x— 1 quixadhal quixadhal 1034002 Oct 12 02:29 rom++-43
-rwxr-x— 1 quixadhal quixadhal 826106 Oct 12 02:24 rom-34
-rwxr-x— 1 quixadhal quixadhal 970046 Oct 12 02:25 rom-41
-rwxr-x— 1 quixadhal quixadhal 968534 Oct 12 02:25 rom-42
-rwxr-x— 1 quixadhal quixadhal 969435 Oct 12 02:26 rom-43


It doesn't really matter…. or rather, if 200K matters to you, you need to find a new hosting provider… but it's interesting to see.
As you'd suspect, this means I have tracked down the gcc errors and will be preparing a patch. I'll also double-check the repository version and see if I missed aything there. I think they're all the result of me moving prototypes around, so consider the last patch I released and this one to be a single entity (they will be if and when they get pushed into svn).
12 Oct, 2008, David Haley wrote in the 100th comment:
Votes: 0
quixadhal said:
I'd be inclined to replace all the "shorts" and "longs" unless there's a good reason not to. The only places it should matter are places where pointers get involved (IE: pointer arithmetic) and evil things that were done when we ran these on the C64, like funky bitmasks that assumed 7-bits or some such.

My general policy is that if I just want a number, it's an int. If I really mean a specific number of bits, I use the correct type, e.g. uint16_t, uint32_t, uint64_t, etc.

Pointer arithmetic should never use numeric types, and should instead use the ptrdiff_t type if you need to store a difference.

As for bitmasks, well, I would use types that specify the number of bits so that I can assume something that will pretty much always be true. :wink:
80.0/181