Login
User Name:

Password:



Register
Forgot your password?
Vote for Us!
auth_update crash
Dec 23, 2017, 10:15 pm
By Remcon
check_tumble
Dec 18, 2017, 7:21 pm
By Remcon
parse description bug
Dec 15, 2017, 10:08 pm
By Remcon
Couple bugs
Dec 12, 2017, 5:42 pm
By Remcon
Bug in disarm( )
Nov 12, 2017, 6:54 pm
By GatewaySysop
LoP 1.46
Author: Remcon
Submitted by: Remcon
LOP 1.45
Author: Remcon
Submitted by: Remcon
LOP Heroes Edition
Author: Vladaar
Submitted by: Vladaar
Heroes sound extras
Author: Vladaar
Submitted by: Vladaar
6Dragons 4.3
Author: Vladaar
Submitted by: Vladaar
Users Online
CommonCrawl, Yahoo!, Bing, Yandex

Members: 0
Guests: 9
Stats
Files
Topics
Posts
Members
Newest Member
478
3,708
19,242
612
Jacki72H
Today's Birthdays
There are no member birthdays today.
Related Links
» SmaugMuds.org » Codebases » AFKMud Support & Development » Code Cleanup stuff
Forum Rules | Mark all | Recent Posts

Code Cleanup stuff
< Newer Topic :: Older Topic > ummm, stuff about code cleanup

Pages:<< prev 1 next >>
Post is unread #1 Oct 2, 2003, 11:43 am   Last edited Nov 24, 2007, 12:28 am by Samson
Go to the top of the page
Go to the bottom of the page

Quixadhal
Conjurer
GroupMembers
Posts398
JoinedMar 8, 2005

Hey, I didn't find an existing thread for this, but here's a nice macro I whipped up while doing some cleanup... one of the common uses for the ever popular temp buffer is code that looks like this:
            sprintf( buf, "%s %s", obj->name, "rename" );
        STRFREE( obj->name );
        obj->name = STRALLOC( buf );

Now, a little while ago I extended the STRALLOC macro (and the underlying str_alloc and str_dup functions) to use varargs, since I hate temp buffers... so it occured to me that the only reason the temp buffer is needed here is because you're using the destination buffer in the creation of the source material. BUT, since the game driver isn't multi-threaded... why keep so many seperate temp buffers that all do the same thing? Hence, this macro was born:
/*
* This code allows the replacement of an allocated pointer
* by a newly allocated pointer which might have used the original
* as a source.
*/
                                                                                                             
#define STRREP(orig, point, ...)                                        
do                                                                      
{                                                                       
if (point)                                                            
{                                                                     
char *strrep_tmp;                                                   
strrep_tmp = (orig);                                                
(orig) = STRALLOC( (point), # );                        
STRFREE(strrep_tmp);                                                
}                                                                     
} while(0)

This nicely saves a pointer to the old value, allocs a new one (doing expansion via vsnprintf of the args), assigns the result to the original pointer, and then frees the original memory.
BTW: the use of varargs macros is a GNU extension to gcc, but it was formalized into the C99 standard.. so any reasonable modern compiler should do it.
Of course, neither of these ideas is necessary... and one can even argue the the resulting code after macro expansion is very slighly more expensive, but it looks much more compact and simple (thus being easier to maintain), and I think the reduction in the number of multi-kilobyte buffers may be worth the few extra CPU cycles.
Oh yeah, in case it isn't obvious... here's what the use of that macro looks like:
STRREP( obj->name, "%s %s", obj->name, "rename" );
       
Post is unread #2 Oct 2, 2003, 12:01 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

On that subject, would the code here do the same thing? It looks like it would, but I can't exactly tell.

Describes as such:


Better than having to use a buffer, strfree, then stralloc, this does it all in one.

Example:
stralloc_printf( obj->short_descr, "A ball created by %s", ch->name);
       
Post is unread #3 Oct 2, 2003, 12:04 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

Would also like some opinions on these 2 threads:

Thread 1
Thread 2
       
Post is unread #4 Oct 2, 2003, 12:24 pm
Go to the top of the page
Go to the bottom of the page

Quixadhal
Conjurer
GroupMembers
Posts398
JoinedMar 8, 2005

Hmmm, I think so!
I didn't think to make it a function, because I was worried about having to cast void * pointers all over the place... but I suppose you really shouldn't be doing STRALLOC or str_dup into anything too weird.
Well, thank you! I think that looks cleaner, although I think I'll change it to return a copy of *pointer, that way you can also do foo = stralloc_printf(NULL, "%d foo", 3) if you like, or if(!stralloc_printf(foo, "%d foo", 3)) bail();

As an interesting side-note... BSD has a function called asprintf/vasprintf which works like vsprintf, but allocates a buffer to hold the result set via malloc. Not useful for most of the mud code, as you'd have to remember to free it later... but interesting.
       
Post is unread #5 Oct 2, 2003, 2:26 pm   Last edited Nov 24, 2007, 12:28 am by Samson
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

So I decided to give this a shot, and spent some time with treasure.c on it. Here's the results, the compile spammed like hell on it for every use:


treasure.c: In function `do_loadrune':
treasure.c:712: warning: passing arg 1 of `stralloc_printf' from incompatible pointer type
treasure.c:713: warning: passing arg 1 of `stralloc_printf' from incompatible pointer type
treasure.c:714: warning: passing arg 1 of `stralloc_printf' from incompatible pointer type


The code:
   stralloc_printf( obj->name, "%s rune", rune->name );
   stralloc_printf( obj->short_descr, "%s Rune", rune->name );
   stralloc_printf( obj->description, "A magical %s Rune lies here pulsating.", rune->name );


So far as I can see I'm using it the way the example says to use it. Any ideas?
       
Post is unread #6 Oct 2, 2003, 3:35 pm
Go to the top of the page
Go to the bottom of the page

Greven
Magician
GroupMembers
Posts204
JoinedMar 5, 2005

The only thing that comes to mind is that you possibly don't have a prototype set up in the right place, I've gotten strange errors like that before with a new function, and the prototype cleared it right up. Of course, I've had no sleep in like 48 hours, so I might be delirious. Sorry.
       
Post is unread #7 Oct 2, 2003, 5:18 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

Prototyped as such in mud.h:

void stralloc_printf( char **pointer, char *fmt, ... );

and the other variant as well:

void strdup_printf( char **pointer, char *fmt, ... );
       
Post is unread #8 Oct 2, 2003, 5:54 pm
Go to the top of the page
Go to the bottom of the page

Greven
Magician
GroupMembers
Posts204
JoinedMar 5, 2005

Ok, let me try to work this out, the function is being expected to be passed a pointer to a pointer, denoted by the **, correct? So, you are instead returning a pointer to a string. So, perhaps,
stralloc_printf( *obj->name, "%s rune", rune->name );
? I dunno, hope this helps though.
       
Post is unread #9 Oct 2, 2003, 6:03 pm
Go to the top of the page
Go to the bottom of the page

Quixadhal
Conjurer
GroupMembers
Posts398
JoinedMar 8, 2005

Hmmm, ah, that's the other bit where using this function IS a little uglier than the macro version. When I'm at work, I'm thinking perl so things don't always click right away
Because the function has to have a pointer to the object you're setting (which is, itself, a char *), you have to pass it the address of the object, ala:
stralloc_printf(&(obj->name), "%s rune", rune->name );

The parens ensure that you are passing the address of obj->name, not trying to double-dereference obj, and then get the name element of that (which is what *obj->name would do). Remember, in C, you a->b is always equivalent to (*a).b.
The macro I made doesn't do that, because it's textually replacing itself with a normal assignment, rather than trying to do the assignment inside the function.

Oh, one other thing I forgot... in all the snippets from that page, please use vsnprintf(buf, MAX_STRING_LENGTH*2, fmt, args); so you actually avoid crashes. Doubling the size of the buffer is just wishful thinking that maybe it'll be enough to catch overflows, whereas using the length-limited version guarentees you'll avoid them (at the risk of truncating your strings if you don't check return values).
       
Post is unread #10 Oct 2, 2003, 6:05 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

treasure.c:592: warning: passing arg 1 of `stralloc_printf' makes pointer from integer without a cast

stralloc_printf( *rune->name, "%s", arg3 );

Nope. That didn't help either.
       
Post is unread #11 Oct 2, 2003, 6:09 pm
Go to the top of the page
Go to the bottom of the page

Greven
Magician
GroupMembers
Posts204
JoinedMar 5, 2005

Sorry, I meant &rune->name, not *rune->name, too tired, because of course your are trying to pass along the address of the memory, not the actual value. My bad.
       
Post is unread #12 Oct 2, 2003, 6:36 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

Well I'll be. That worked. See, I knew someone more savvy with string pointers would be able to spot this. I bet the example page stripped the & for some reason. When I went and did that with my two examples here, it worked like a charm. I got the results I expected from my random treasure test command. So I think this will do nicely now.
       
Post is unread #13 Oct 3, 2003, 8:29 am   Last edited Nov 24, 2007, 12:28 am by Samson
Go to the top of the page
Go to the bottom of the page

Quixadhal
Conjurer
GroupMembers
Posts398
JoinedMar 8, 2005

Just as a last little closure to the new str functions, here's the version I settled on, which seems to work quite niecly:

char *stralloc_printf( char **orig, char const *fmt, ... )
{
    va_list arg;
    char str[MSL*4];
    char *tmp = NULL;
 
    va_start(arg, fmt);
    vsnprintf(str, MSL*4, fmt, arg);
    va_end(arg);
 
    tmp = str_alloc(str);
 
    if( orig )
    {
        if (*orig)
            STRFREE(*orig);
        *orig = tmp;
    }
 
    return tmp;
}


If anyone remembers the const flag better than I do, my intent was to make fmt be a const pointer to a string, rather than a pointer to a const string. I *think* that's right, but feel free to correct me!
       
Post is unread #14 Oct 3, 2003, 10:25 am
Go to the top of the page
Go to the bottom of the page

Quixadhal
Conjurer
GroupMembers
Posts398
JoinedMar 8, 2005

Heh, so after doing many replacements in the source and noting that everything appeared to be happy, I decided to get valgrind and run it through that. Lo and behold, leftover sloppiness on the part of the diku/smaug team abounds!

How so, you say? How could the gods of eldar times be sloppy???

It would appear that in many places malloc is used instead of calloc when allocating new structures. (Of course, by many, I mean about 3 + hashstr.c) Since malloc doesn't clear memory before it hands it to you, lots of structures start life filled with garbage. When our spiffy new routine checks to see if a pointer is NULL, it finds that it isn't, and so calls str_free on it. str_free also checks NULL's, but even though it's garbage that doesn't point at OUR memory, it ain't NULL... so we attempt to free stuff that isn't ours.

But why doesn't it crash instantly if this be true?

Ah, that's becaue str_free won't call free() unless ptr->links is 0.... but guess what, it's not 0... it's filled with garbage about 98% of the time. So what that really means is, I suspect, strings which could have been freed never have been, because their reference counts didn't start at 0, they started at a random number between 0 and 65535.

So, word to the wise... always initialize every variable, dynamicly allocated or otherwise. It doesnt' take much horsepower unless it's a gigantic 64K buffer or more, and it makes debugging a lot cleaner!

BTW: this shows up in valgrind as an invalid read of size 2, and as multiple bad pointer free attempts at shutdown.
       
Post is unread #15 Oct 4, 2003, 3:11 am
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
JoinedJan 1, 2002

Ok, hrm. So I see hashstr.c does indeed use malloc. Wouldn't changing that to use calloc solve 98% of the problems in one short stroke? I'd have to look at how hashstr.c is working, but I can't see why it wouldn't work to be switched to calloc since the man page says it initializes stuff to 0's first.
       
Post is unread #16 Oct 4, 2003, 2:12 pm
Go to the top of the page
Go to the bottom of the page

Quixadhal
Conjurer
GroupMembers
Posts398
JoinedMar 8, 2005

Yup, that will catch a great many of them. The leftover places will most likely be places where a pointer is left inside a sturcture that's not initialized, or non-dynamic variables that go out of scope while holding a pointer to a str_dup or str_alloc's structure (or any dynamic one for that matter)...

That's what I was trying to say by a gazillion places like 3 others and then hashstr.c

Be thankful you're not coding in perl, at least we have the ability to find where memory goes... perl has wonderful things like auto-vivication of arrays (if you access $a[13] and there were only 12 elements, it will extend the array and return an undef... thus eating up memory rather than producing a seg-fault). That's great for simplicity of coding, but a nightmare if you care about memory or performance.
       
Pages:<< prev 1 next >>