Login
User Name:

Password:



Register

Forgot your password?
Changes list / Addchange
Author: Khonsu
Submitted by: Khonsu
6Dragons mp3 sound pack
Author: Vladaar
Submitted by: Vladaar
AFKMud 2.2.3
Author: AFKMud Team
Submitted by: Samson
SWFOTEFUSS 1.5
Author: Various
Submitted by: Samson
SWRFUSS 1.4
Author: Various
Submitted by: Samson
Users Online
AhrefsBot, Bing, Google

Members: 0
Guests: 27
Stats
Files
Topics
Posts
Members
Newest Member
488
3,788
19,631
595
Khonsu

Today's Birthdays
There are no member birthdays today.
» SmaugMuds » General » General Discussions » How do we handle the errors c...
Forum Rules | Mark all | Recent Posts
How do we handle the errors caused by gcc4.2? (14 Votes)
Update to const char *argument and adjust all the rest of the code accordingly.  57.14% - 8 votes

Modernize and update to std::string throughout the code.  42.86% - 6 votes

Other. (Post an explanation of your alternative solution.)  0% - 0 votes

How do we handle the errors caused by gcc4.2?
< Newer Topic :: Older Topic >

Pages:<< prev ... 2, 3, 4, 5, 6 ... next >>
Post is unread #61 Jul 25, 2008 5:59 pm   
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

 
Samson said:

I'll take another look right now but I don't recall seeing any "raw" links.

It's at the top-left of the page, in a list of 5 options: main, log, inventory, swap diff, raw.

Samson said:

I also don't see how branching one thing, putting "my" code on top of it, merging the const-fix, then recommitting would be at all easy for those of us who know nothing of how bzr works.

Well, my experience with patch has always been a little, err, patchy. (groan) The workflow with bzr is very easy:

bzr branch (http://... stock 1.9)
move your code on top of it
bzr commit (to make your customized code the current version)
bzr merge (http://... const-fix 1.9)
(deal with conflicts, like in any other VCS, if any arise)

Samson said:

And thinking about it, what code is there to merge? 1.9 hasn't been touched in quite some time, mainly because we were waiting on this fix to be ready but also because as I said, nobody *cough*Kayle*cough* has given me anything ready for adding in anyway.

In your case, dealing with the stock version, I guess there is nothing to merge and you can use the const-fix code exactly as is. I was thinking of people who have their own code bases, because they are doing a three-way merge of sorts:

     Stock FUSS 
               \
 Const-fix FUSS ---  =  customized FUSS with const-fix applied
               /
Customized FUSS


Letting bzr take care of it makes life easier.

Basically, this bzr method lets you easily combine two separate "patches":
stock --> customized
stock --> const-fix

Doing this by hand is painful at best, and so tedious as to be impractical at worst, depending on the size of the changes.

Post is unread #62 Jul 25, 2008 6:01 pm   
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,685
JoinedJan 1, 2002

 
Gah, I edited my last post before you posted your reply :)

Post is unread #63 Jul 25, 2008 6:04 pm   
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

 
Samson said:

So unless anyone can think of some REALLY good reason why this shouldn't go live as a 1.9 update I'd love to hear it now....

Well, I can think of a reason, but I'm not sure if it's a "REALLY good reason". :tongue: It is something to be aware of, though. The patch will change the way people work with strings. All do_cmds now take const string args which means that you can't just edit them anymore like you could in the past. If somebody really needs to edit the string, they'll have to copy it to a local string first. Now, it turns out that this wasn't done much in practice (looking at the diff will show this). But it could confuse people who aren't used to dealing with const strings. Probably a rather minor concern, but still, we should probably be aware of this in case somebody has questions about it.

Post is unread #64 Jul 25, 2008 6:32 pm   
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,685
JoinedJan 1, 2002

 
Well that's to be expected, isn't it? Other than that I can't see any reason not to go forward with this as the official 1.9 since the entire point was to fix the problem for future Smaug users on 4.2.

Post is unread #65 Jul 25, 2008 6:54 pm   
Go to the top of the page
Go to the bottom of the page

InfiniteAxis
Off the Edge of the Map
GroupAdministrators
Posts1,200
JoinedMar 21, 2006

 
Samson said:

And thinking about it, what code is there to merge? 1.9 hasn't been touched in quite some time, mainly because we were waiting on this fix to be ready but also because as I said, nobody *cough*Kayle*cough* has given me anything ready for adding in anyway.


*cough*What about this one?*cough*

Post is unread #66 Jul 27, 2008 4:18 pm   Last edited Jul 27, 2008 4:20 pm by David Haley
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

 
The URL has changed; please use http://w3.binarygoblins.com:8088/. Sorry for the inconvenience of the changes. I might make one more change in the future to make the URL friendly, but on the other hand I doubt this page will be "permanent" so it might not matter.

Post is unread #67 Jul 27, 2008 10:43 pm   
Go to the top of the page
Go to the bottom of the page

Keberus
Conjurer
GroupFUSS Project Team
Posts341
JoinedJun 4, 2005

 
David, would it be possible for you to upload the smaugfuss const fixed codebase as well as the diff files here for easier access. I don't know if anyone else is having problems but everytime I click on Repository I get:


Index of /~david/bzr/fussproject/SmaugFUSS/stable-const-fix
Name Last modified Size Description
--------------------------------------------------------------------------------

Parent Directory -
webserve.cgi 22-Jul-2008 11:01 1.7K

--------------------------------------------------------------------------------
Apache/2.2.6 (Debian) DAV/2 mod_python/3.3.1 Python/2.4.4 PHP/4.4.4-9 mod_ruby/1.2.6 Ruby/1.8.6(2007-06-07) mod_ssl/2.2.6 OpenSSL/0.9.8e WebAuth/3.5.4 mod_perl/2.0.3 Perl/v5.8.8 Server at w3.binarygoblins.org Port 8080


Anyways, it would be nice if there was an easy way to download the diff and all the source files at once.

Please and thank you,

Keberus

Post is unread #68 Jul 27, 2008 10:53 pm   
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

 
The repository link is the bzr repository; you don't want that unless you're using bzr. I posted the link to the diff a while back; here it is if you missed it. (Note that it might take about a minute to generate as the server it runs on is a little slow.) If you click on "raw" in the upper left, you'll get a "plain" diff that can be applied via 'patch' or similar.

As for source files, I did upload a .tgz here but I don't think the file has been approved yet. That said, the patch was integrated with the main distribution, so you should be able to just download the normal FUSS package and have all the sources that way.

If you're willing to use bzr, that is the easiest way to download both the current source and source history. The command is: bzr branch , where is the "Repository" link that you clicked on already.

Post is unread #69 Jul 27, 2008 11:59 pm   
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,685
JoinedJan 1, 2002

 
The file wasn't approved because the main distribution now has those changes applied so there was no need for a second confusing source package to be here. If you'd like to upload a diff of the changes that's probably useful. Although I have the diff here and could upload that if need be.

Post is unread #70 Jul 28, 2008 5:26 am   
Go to the top of the page
Go to the bottom of the page

kiasyn
Magician
GroupMembers
Posts121
JoinedJun 30, 2006

 
I did notice this memory leak in the diff:

+   {
+      char* newrank = str_dup(argument);
+      smash_tilde( newrank );
+      ch->pcdata->rank = str_dup( newrank );
+   }


and this:
+

+   char* tmp = strdup(argument);

+   if( tmp[0] != '\0' )

+      tmp[0] = UPPER( tmp[0] );

+

+   class_table[rcindex]->who_name = STRALLOC( tmp );

+

+   free(tmp);

+


not a bug per-say, but smaug practices IIRC is to use DISPOSE not free. this might confuse some people.

this:
+void do_name( CHAR_DATA* ch, const char* argument)
 {
+   char ucase_argument[MAX_STRING_LENGTH];
    char fname[1024];
    struct stat fst;
    CHAR_DATA *tmp;
@@ -3265,15 +3266,16 @@
       return;
    }
-   argument[0] = UPPER( argument[0] );
+   mudstrlcpy( ucase_argument, "argument", MAX_STRING_LENGTH );
+   ucase_argument[0] = UPPER( argument[0] );


think it should just be argument, not "argument"

+const char* smash_tilde( const char *str )
+{
+    static char buf[MAX_STRING_LENGTH];
+    mudstrlcpy( buf, str, MAX_STRING_LENGTH );
+    return buf;
+}


I don't see how this is actually smashing tildes.

+void interpret( CHAR_DATA * ch, const char* argument)
+{
+    char* temp = strdup(argument);
+    interpret(ch, temp);
+    free(temp);
+}


this looks like trouble just waiting to happen



just a quick scan over the diff :)

Post is unread #71 Jul 28, 2008 8:02 am   
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

 
I'll take a look at these in an hour or so. What is the problem with the const version of interpret? It's meant to be a wrapper around interpret when you only have a const string.

Post is unread #72 Jul 28, 2008 10:26 am   Last edited Jul 28, 2008 10:28 am by David Haley
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

 
Here is the diff to account for the problems Kiasyn found.

=== modified file 'src/act_wiz.c'
--- src/act_wiz.c       2008-07-22 17:51:34 +0000
+++ src/act_wiz.c       2008-07-28 15:13:20 +0000
@@ -763,7 +763,7 @@
    {
       char* newrank = str_dup(argument);
       smash_tilde( newrank );
-      ch->pcdata->rank = str_dup( newrank );
+      ch->pcdata->rank = newrank;
    }
    send_to_char( "Ok.\r\n", ch );
    return;
@@ -7737,7 +7737,7 @@
 
    class_table[rcindex]->who_name = STRALLOC( tmp );
 
-   free(tmp);
+   DISPOSE(tmp);
 
    xCLEAR_BITS( class_table[rcindex]->affected );
    class_table[rcindex]->attr_prime = 0;

=== modified file 'src/comm.c'
--- src/comm.c  2008-07-22 17:55:17 +0000
+++ src/comm.c  2008-07-28 15:16:16 +0000
@@ -3266,7 +3266,7 @@
       return;
    }
 
-   mudstrlcpy( ucase_argument, "argument", MAX_STRING_LENGTH );
+   mudstrlcpy( ucase_argument, argument, MAX_STRING_LENGTH );
    ucase_argument[0] = UPPER( argument[0] );
 
    if( !check_parse_name( ucase_argument, TRUE ) )

=== modified file 'src/db.c'
--- src/db.c    2008-07-22 17:51:34 +0000
+++ src/db.c    2008-07-28 15:17:28 +0000
@@ -3946,6 +3946,7 @@
 {
     static char buf[MAX_STRING_LENGTH];
     mudstrlcpy( buf, str, MAX_STRING_LENGTH );
+    smash_tilde(buf);
     return buf;
 }
 


Thanks Kiasyn :smile:

I looked again and didn't see any other issues, but then again in a change of this size there might be a few. number_argument and warn_in_prog could use more some more eyeballs -- I only glanced through them now but it's the kind of code that could cause trouble.


Here is a link to the full diff from stock FUSS to the version with the const fix (and above fixes).

Post is unread #73 Jul 29, 2008 7:20 pm   
Go to the top of the page
Go to the bottom of the page

Keberus
Conjurer
GroupFUSS Project Team
Posts341
JoinedJun 4, 2005

 
Samson said:

The file wasn't approved because the main distribution now has those changes applied so there was no need for a second confusing source package to be here. If you'd like to upload a diff of the changes that's probably useful. Although I have the diff here and could upload that if need be.


That's cool, I mainly wanted the source to peek and poke at, I didn't know it had already been integrated before I posted. Guess I should've checked the files section a bit more often. Anyways thanks for the update.

Post is unread #74 Aug 17, 2008 4:37 pm   Last edited Aug 17, 2008 4:37 pm by Kayle
Go to the top of the page
Go to the bottom of the page

InfiniteAxis
Off the Edge of the Map
GroupAdministrators
Posts1,200
JoinedMar 21, 2006

 
So I'm going over this installing it in my personal base, (by hand, because so much shit's been rearranged, that well, patching would have been a nightmare. :D) And I noticed this:

const char* smash_tilde( const char *str )
{
    static char buf[MAX_STRING_LENGTH];
    mudstrlcpy( buf, str, MAX_STRING_LENGTH );
    smash_tilde(buf);
    return buf;
}

char* smash_tilde_copy( const char *str )
{
    char* result = strdup(str);
    smash_tilde(result);
    return result;
}


And I have one question. How does this do what smash_tilde is supposed to do exactly?

Smash_tilde used to look like:
void smash_tilde( char *str )
{
   for( ; *str != '\0'; str++ )
      if( *str == '~' )
         *str = '-';

   return;
}


I'm not seeing how they work the same. >.>

Post is unread #75 Aug 17, 2008 4:50 pm   
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,685
JoinedJan 1, 2002

 
The function is exploiting overloading which is possible with C++. The const char version of smash_tilde copies the string into a temp buffer. That temp buffer is sent through the original smash_tilde function and comes out the other end already modified. The modified result is what's returned from the overloaded smash_tilde.

I don't know where smash_tilde_copy came from but it should never be used. As written it has a memory leak in it.

Post is unread #76 Aug 17, 2008 4:53 pm   
Go to the top of the page
Go to the bottom of the page

InfiniteAxis
Off the Edge of the Map
GroupAdministrators
Posts1,200
JoinedMar 21, 2006

 
Oh, I get it now. I replaced the original with this one instead of placing it after. Heh. *goes back and fixes it*

Post is unread #77 Aug 17, 2008 7:44 pm   
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

 
Samson said:

I don't know where smash_tilde_copy came from but it should never be used. As written it has a memory leak in it.

No, it doesn't; it means that the person calling it is responsible for freeing the memory. That's why the function has a different name: the suffix "_copy" indicates that, well, a copy is being made. :wink:

Post is unread #78 Aug 17, 2008 8:11 pm   
Go to the top of the page
Go to the bottom of the page

InfiniteAxis
Off the Edge of the Map
GroupAdministrators
Posts1,200
JoinedMar 21, 2006

 
That seems kinda pointless to me... There a reason it's done?

Post is unread #79 Aug 17, 2008 9:54 pm   
Go to the top of the page
Go to the bottom of the page

InfiniteAxis
Off the Edge of the Map
GroupAdministrators
Posts1,200
JoinedMar 21, 2006

 
So I finally finished updating my base to include this. And after doing it by hand, I *think* I understand it. I guess we'll see when I go to fix the code for my base that isn't covered by that diff. :P

Post is unread #80 Aug 17, 2008 10:42 pm   Last edited Aug 17, 2008 10:42 pm by David Haley
Go to the top of the page
Go to the bottom of the page

David Haley
Sorcerer
GroupMembers
Posts903
JoinedJan 29, 2007

 
Sometimes you don't have a char* available, but want to call str_dup and keep the result. But, you don't want to depend on the static buffer, because that's dangerous (for various reasons). Also, you need the result to be a string that you own (i.e. a char*, not a const char*). So, instead of copying the string and then calling smash_tilde, you call the version that copies for you and then free it. Whether or not this is useful to you is up to you; either way (copying or static buffer) has its dangers. Speaking of, hset needs to be updated to dispose of the string returned by smash_tilde_copy. :wink: In any case, the better solution would be to use some kind of smart string object like std::string, but that's another discussion for another day...

Pages:<< prev ... 2, 3, 4, 5, 6 ... next >>