Could be what it is but I'm still running 3.2.3 as opposed to the current version of 4.3.2 and everything is running fine with the latest Roguemud build.

Hmm, are you saying that double-frees would not have caused crashes in the past? (For what it's worth, the C libraries like glibc aren't part of the OS, which handles things like memory pages and segmentation faults, not the C side of memory management.)

Well, I'm about half way through porting the OLC 1.81 patch to work with RaM Fire.

I just ran into something rather odd, and thought I would bring it up. Check these lines out starting at 1512 in db.c:

mob = create_mobile( pMobIndex );

                /*
                 * Check for pet shop.
                 */
                {
                    ROOM_INDEX_DATA        *pRoomIndexPrev = NULL;

                    pRoomIndexPrev = get_room_index( pRoomIndex->vnum - 1 );
                    if ( pRoomIndexPrev != NULL
                         && IS_SET( pRoomIndexPrev->room_flags, ROOM_PET_SHOP ) )
                        SET_BIT( mob->act, ACT_PET );
                }

What on earth are those brackets even doing?? They are like that in the stock Rom2.4b6. Kinda strange… I can't see the purpose… am I overlooking something?

They're creating a new stack scope. Sometimes it's convenient to do that if you want variables local to a specific part of the function. Also, some compilers only allowed declarations at scope open, so you would do things like that to avoid having to declare everything at the top of the function.

Thanks for the quick response.

Interesting. I've never seen (or at least noticed) that being done before. So the variables declared in there go out of scope after the closing brace, and are no longer available to the function?

I've only got 4 hunks left in db.c to port, and after that the rest of the OLC patch is smooth sailing (comparatively).

Yup, that's correct. Any time braces open, actually, a new scope is created, although normally people only uses braces after some kind of control-flow statement (if, while, etc.).

I usually like declaring variables in the inner-most scope because it makes the code cleaner, and it makes it harder to accidentally refer to a variable in a place you shouldn't.

Alright, all db.c hunks are finally done. The rest of the patch should be pretty easy to finish up, so I'll get that up here in a day or so.

Quix,

I like whatever you are using for auto-formatting. It really improves the readability of the code. There is one thing it does I don't agree with, though:

In switches, it indents the case statements from the original level of the switch, like this:

switch ( letter = fread_letter( fp ) )
        {
            default:
                proper_exit( MUD_HALT, "Load_specials: letter '%c' not *MS.", letter );

            case 'S':
                return;

            case '*':
                break;

            case 'M':
                pMobIndex = get_mob_index( fread_number( fp ) );
                pMobIndex->spec_fun = spec_lookup( fread_word( fp ) );
                if ( pMobIndex->spec_fun == 0 )
                {
                    proper_exit( MUD_HALT, "Load_specials: 'M': vnum %d.",
                                 pMobIndex->vnum );
                }
                break;
        }

I don't like this because it leaves an indentation level at the far side that doesn't have braces to match it. When I write case statements, I always indent like this:

switch ( letter = fread_letter( fp ) )
        {
        default:
            proper_exit( MUD_HALT, "Load_specials: letter '%c' not *MS.", letter );

        case 'S':
            return;

        case '*':
            break;

        case 'M':
            pMobIndex = get_mob_index( fread_number( fp ) );
            pMobIndex->spec_fun = spec_lookup( fread_word( fp ) );
            if ( pMobIndex->spec_fun == 0 )
            {
                proper_exit( MUD_HALT, "Load_specials: 'M': vnum %d.", pMobIndex->vnum );
            }
            break;
        }

Then the closing brace of the switch matches the indentation levels of the case statements. Just works out better, and as far as I know this is standard. Hoping that you agree on this point, can you configure the formatting program you use to do this? Just wondering if it is an option… we should wait to enact any changes until we apply this OLC patch to the Fire branch obviously.

I've always put the case statements indented after the brace, not on the same level. It makes it much more obvious that they are conditions inside the greater switch statement, as opposed to statements on the same level as the switch statement.

Correct. All free() used to do is mark that segment of memory as available for reallocation… if nothing had yet reallocated it, it wouldn't always cause a problem since the chunk you just tried to free (again) wasn't in the allocated-memory list. Now, trying to free something that doesn't exist raises the same error as trying to free something you don't own.

I would argue that point, actually. Technically, if we're talking about the kernel, you're correct of course.

Here's my reasoning. Have you ever nuked libc when trying to do an upgrade? It's pretty hard to do nowadays, but it wasn't always so difficult to do. As a fun experiment, try moving your libc.so file to some other filename and see just how many things are broken. The OS itself is written in C (almost 99% of it), and almost every command you use on a daily basis, every daemon that runs, is linked against libc. Once you remove that file, all bets are off as to what will keep running and what will die.

It didn't used to be that way, of course. Back when standards were understood, rather than approved by committee, there was a directory called sbin. It stood for "static binaries", and all the tools in there were linked statically so that when you DID nuke libc, you had some things to use in recovery. Go ahead and use "ldd" on stuff in there now… it's all linked against libc.

Because of that, I disagree that libc (glibc in the case of linux) is NOT part of the OS, since I don't believe anything beyond the kernel itself can operate without it. Even /sbin/init is linked against libc, and that is by definition, the first process.

Actually, I do prefer them to be indented in a level. I think it looks cleaner, and follows the logic of the case structure more accurately. In the above example, pMobIndex isn't being assigned inside the switch statement, it's being assigned inside the case statement, which itself is inside the switch statement. Doing it the other way looks like:

while(foo)
{
if(bar)
{
    booga();
}
else if(pfft)
{
    bongo();
}
}

I actually take a tip from python here. If you indent things properly, the braces should be optional. If you can't remove the braces and still tell what code is inside what constructs, the indentation has failed.



Oh, and sorry I missed the meeting tonight! I've been battling one of these ugly winter colds that has me awake for 2 hours and then asleep for 4, and it won that round. :(

So, if you guys did get together and chat, feel free to post the results somewhere around here. When I was last concious, I was working on hybridizing the colour code I had from Smaug and Lope's to make something a little more solid that still follows the Lope syntax.

I'm just wondering why, instead of taking a cue from Python or personal preferences, didn't you just pick a style common to internet projects, like indent -gnu, indent -kr, or indent -orig?
GNU, Berkeley, and K&R styles (as well as examples in the C standards document) keep labels and case labels indented at the block level. I suspect some of the reasons for not indenting labels is that all code then has to be indented two levels to make labels stand out, and that labels don't introduce a new level of scope like other constructs.
Anyway another option, and probably a better one IMHO, is simply not to really care so much lest one turn off contributors by style lectures.

Are you sure that the memory allocation routines always run those checks and it's not some kind of debugging option? It seems that that could be a performance hit, and if you were compiling with optimizations you wouldn't want every deallocation to incur those extra checks.

Now that you mention it, it's true that double-frees used to cause havoc indirectly by having things stomp over each other, but not immediately crash. (That said, the immediate crashing doesn't always happen for me, hence my question above.)

I suppose that if by OS you also mean the system utilities and daemons, then sure, libc is part of it. By OS I meant just the kernel, and definitely not the daemons and various commands I use. In principle, you could build another runtime environment on top of the kernel, using some other language's runtime and so forth. I can see how you might include various fundamental libraries like libc as part of the OS, though.


I agree, so let's just leave the labels indented after the switch statement. :tongue:

Alright, I won't complain about the switch format anymore. Just noticed it was different from what I always did, which I thought was standard formatting.

Anyway, I had some more time this morning to hack at the OLC patch, which will probably be finished tonight.

I did run into some more confusing code, can you guys clarify what is with the args(…) in these last three declarations?

int                     position_lookup( const char *name );
 int                     sex_lookup( const char *name );
 int                     size_lookup( const char *name );
int                     flag_lookup	args( (const char *, const struct flag_type *) ); 
HELP_DATA*              help_lookup	args( (char *) );
HELP_AREA*              had_lookup	args( (char *) );

Am I safe to pull the arguments out of that args(…) and make these function declarations appear like one would expect?

True enough on the indent. I mean, when I change things *I* run the code through indent… I figure if one of the options selected really bugs someone, they're welcome to do the same. :)

As for glibc/malloc, I believe it is an option, but it's one that is enabled by default. Recently, I was looking at the malloc man page (in another thread) and there's an environmental variable that can be set to 0, 1 or 2. If you set it to 1 (the default), it halts program execution on memory corruption checks. If you set it to 0, you get the old dinosaur behavior of it not running said checks (and thus crashing a while later), if you set it to 2, it forces an abort(), which is more useful than 1 – since it will generate a core dump or allow you to run in the debugger and view the stack frame.

Those are old-fashioned macros which people used to declare the function arguments back in the pre-ANSI (K&R) days, when compilers wouldn't always accept argument lists at all. On krufty systems, args() would become a no-op so your functions just got declared as existing, nothing else.

Please do get rid of them. They're in the same boat as VMS support and other old friends. :)

Oh, you ARE making everything use (const char *) wherever possible, right? gcc 4.2.x is unhappy every passing a const-declared string into a function that isn't const declared, and it'll have a fit if you're not careful.

For what it's worth, from experience fixing a few SMAUG-based codebases for constness, the vast majority of strings should be const. There are just a few places where you actually need mutable strings.

Alright then, I'll get rid of the args(…) malarkey everywhere it appears.

I haven't been replacing "char *" with "const char *", but I'll go back and do that.

Should I also replace all "sh_int" variables with regular "int"?

I have been replacing bug(…) with log_error(…), send_to_char(…) with ch_printf(…), and any bug(…); exit(1); combo with proper_exit(MUD_HALT, …).

Alright, the OLC 1.81 patch is finally completed. (HUZZAH!)

I am now beginning the process of modifying the individual OLC source files to use appropriate RaM functions, and also to translate messages to english.

Heh. If you need some help on the translations, there's an already translated version -> http://www.mudbytes.net/index.php?a=file...

Thanks for the reference, Davion. Thanks to Google Language, I'm fluent in spanish (while connected to the internet). =) I've got to replace a ton of bug(…) and print_to_char(…) etc, etc, so the translation isn't much more on top of that.

Quix,

I hope you feel better… I bet if you port this patch bit to work with your RaM Makefile, it'll cure your ailments!

diff -ur old/Makefile.linux new/Makefile.linux
— old/Makefile.linux	Mon Sep 14 17:46:16 1998
+++ new/Makefile.linux	Mon Sep 14 17:46:53 1998
@@ -9,7 +9,8 @@
           alias.o ban.o comm.o const.o db.o db2.o effects.o fight.o flags.o \
 	  handler.o healer.o interp.o note.o lookup.o magic.o magic2.o \
 	  music.o recycle.o save.o scan.o skills.o special.o tables.o \
-	  update.o
+ 	  update.o mob_cmds.o mob_prog.o olc.o olc_act.o olc_save.o bit.o \
+	  mem.o string.o olc_mpcode.o hedit.o
 
 rom: $(O_FILES)
 	rm -f rom

Haha, but seriously, I'm not familliar with much of the syntax you used in the RaM Makefile, so you'd be best for this job. Its just the standard OLC 1.81 snippet.

Let me know, I'm hoping to RaM compiling w/ OLC tonight. I'll hack it if I have to.

[EDIT: ] Obviously you can just hand me a completed Makefile, the patch isn't going to be used outside this development phase.

Don't worry about the Makefile… I got anxious and couldn't wait to try to compile RaM Fire with OLC. As you can imagine, I've run into lots of juicy fixes that the OLC code requires to be compliant. Its a joy to try to use smash_tilde(…) on strings passed in when everything is const correct. ;)

It's coming along nicely though.

Here is the Makefile I'm using (dependencies may change yet):

#
# RAM $Id: Makefile 62 2008-11-17 04:58:19Z quixadhal $
#

CC      	= g++

CRYPT		= #-DNOCRYPT		# Uncomment to use clear-text passwords
RAND		= #-DOLD_RAND		# Uncomment if your random number generator is broken
SOCIAL		= #-DSOCIAL_DEBUG	# Uncomment for debugging of the socials
PLAYERLIST      = #-DPLAYER_LIST        # Uncomment for player list features

# Comment out the -Wmissing-format-attribute flag if you're using gcc 2.95
# or upgrade!
#
# The W_CONLY set of flags are only valid when using C, not for C++
# -Wmissing-declarations is C only under g++ 4.2.x and older!

W_ERROR         = -Werror
W_ANSI          = #-pedantic
W_UBER          = -Wall
W_FORMAT        = -Wformat -Wformat-security -Wmissing-format-attribute
W_MESSY         = -Wmissing-braces -Wparentheses -Wshadow -Wredundant-decls
W_TYPE          = -Wcast-qual -Wcast-align -Wchar-subscripts -Wreturn-type -Wswitch -Wwrite-strings
W_EXTRA         = -Wunused -Wuninitialized #-Wunreachable-code
W_NITPICK       = -Wpointer-arith -Winline
ifeq ($(CC), gcc)
W_CONLY		= -Wmissing-declarations -Wmissing-prototypes -Wstrict-prototypes 
endif

WARN		= $(W_ERROR) $(W_ANSI) $(W_UBER) $(W_FORMAT) $(W_MESSY) $(W_TYPE) $(W_EXTRA) $(W_NITPICK) $(W_CONLY)

OPT	    	= -O3 				# You trust the compiler, don't you?
DEBUG		= -g				# Just enough to fix the occasional crash
#DEBUG		= -ggdb3			# Full debugging with ALL defined symbols!
PROF		= #-pg				# Enable profiling via gprof

OPTIONS		= $(OPT) $(DEBUG) $(PROF) $(CRYPT) $(RAND) $(SOCIAL) $(PLAYERLIST)

C_FLAGS 	= $(WARN) $(OPTIONS)
L_FLAGS 	= $(DEBUG) $(PROF)

LIBS		= 				# Uncomment if you're using Linux
#LIBS		= 				# Uncomment if you're using BSD or OS X
#LIBS		= -lsocket -lresolv -lnsl	# Uncomment if you're using Solaris

O_FILES		= act_comm.o act_info.o act_move.o act_obj.o act_wiz.o \
		  alias.o ban.o bit.o bug.o comm.o const.o \
		  db.o fight.o handler.o healer.o hedit.o \
		  interp.o magic.o mem.o mob_cmds.o mob_prog.o note.o \
		  olc.o olc_act.o olc_mpcode.o olc_save.o playerlist.o \
		  random.o save.o sha256.o skills.o special.o string.o \
		  strings.o tables.o update.o

ram: $(O_FILES)
	@rm -f $@
	$(CC) $(L_FLAGS) -o $@ $(O_FILES) $(LIBS)
	@ls -l $@

%o : %c
	$(CC) $(C_FLAGS) -c $< -o $@

tags :
	@rm -f $@
	@ctags *.[ch]
	@ls -l $@

clean:
	@rm -f *.o

spotless:
	@rm -f *.o tags ram gmon.out

dep:
	@find . -name \*.c -a -type f | sort | xargs -P 1 -r $(CC) $(OPTIONS) -MM

#— Dependancies go below here —

act_comm.o: act_comm.c merc.h strings.h random.h tables.h db.h interp.h \
  magic.h act.h
act_info.o: act_info.c sha256.h merc.h strings.h random.h tables.h db.h \
  interp.h magic.h act.h
act_move.o: act_move.c merc.h strings.h random.h db.h interp.h magic.h \
  act.h
act_obj.o: act_obj.c merc.h strings.h random.h db.h interp.h magic.h \
  act.h
act_wiz.o: act_wiz.c merc.h tables.h strings.h random.h db.h interp.h \
  magic.h special.h act.h
alias.o: alias.c merc.h strings.h interp.h
ban.o: ban.c merc.h strings.h db.h interp.h
bit.o: bit.c merc.h interp.h tables.h
bug.o: bug.c merc.h db.h
comm.o: comm.c sha256.h merc.h tables.h strings.h db.h act.h interp.h \
  magic.h
const.o: const.c merc.h magic.h interp.h
db.o: db.c merc.h strings.h random.h tables.h act.h db.h interp.h magic.h \
  special.h
fight.o: fight.c merc.h strings.h random.h db.h interp.h act.h magic.h
handler.o: handler.c merc.h strings.h random.h tables.h db.h act.h \
  interp.h magic.h
healer.o: healer.c merc.h strings.h random.h interp.h magic.h
hedit.o: hedit.c merc.h interp.h tables.h db.h olc.h strings.h
interp.o: interp.c merc.h strings.h random.h db.h act.h interp.h
magic.o: magic.c merc.h strings.h random.h db.h act.h interp.h magic.h
mem.o: mem.c merc.h tables.h strings.h
mob_cmds.o: mob_cmds.c merc.h interp.h mob_cmds.h
mob_prog.o: mob_prog.c merc.h interp.h tables.h
note.o: note.c merc.h strings.h db.h interp.h tables.h
olc.o: olc.c merc.h interp.h tables.h olc.h
olc_act.o: olc_act.c merc.h interp.h tables.h db.h olc.h
olc_mpcode.o: olc_mpcode.c merc.h interp.h tables.h db.h olc.h
olc_save.o: olc_save.c merc.h tables.h olc.h
playerlist.o: playerlist.c merc.h strings.h db.h
random.o: random.c merc.h random.h
save.o: save.c merc.h strings.h tables.h db.h magic.h
sha256.o: sha256.c sha256.h
skills.o: skills.c merc.h strings.h random.h db.h interp.h magic.h
special.o: special.c merc.h strings.h random.h db.h act.h interp.h \
  magic.h special.h
string.o: string.c merc.h interp.h tables.h olc.h
strings.o: strings.c merc.h strings.h
tables.o: tables.c merc.h strings.h db.h interp.h tables.h
update.o: update.c merc.h random.h db.h act.h interp.h magic.h