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, Yandex, Yahoo!, DotBot, Exalead

Members: 0
Guests: 11
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 » 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, 3: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, 4: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

Gah, I edited my last post before you posted your reply :)
       
Post is unread #63 Jul 25, 2008, 4: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, 4:32 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 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, 4:54 pm
Go to the top of the page
Go to the bottom of the page

Kayle
Off the Edge of the Map
GroupAdministrators
Posts1,195
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, 2:18 pm   Last edited Jul 27, 2008, 2: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, 8: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, 8: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 <repo>, where <repo> is the "Repository" link that you clicked on already.
       
Post is unread #69 Jul 27, 2008, 9:59 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
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, 3: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, 6: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, 8:26 am   Last edited Jul 28, 2008, 8: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, 5: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, 2:37 pm   Last edited Aug 17, 2008, 2:37 pm by Kayle
Go to the top of the page
Go to the bottom of the page

Kayle
Off the Edge of the Map
GroupAdministrators
Posts1,195
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, 2:50 pm
Go to the top of the page
Go to the bottom of the page

Samson
Black Hand
GroupAdministrators
Posts3,639
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, 2:53 pm
Go to the top of the page
Go to the bottom of the page

Kayle
Off the Edge of the Map
GroupAdministrators
Posts1,195
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, 5: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, 6:11 pm
Go to the top of the page
Go to the bottom of the page

Kayle
Off the Edge of the Map
GroupAdministrators
Posts1,195
JoinedMar 21, 2006

That seems kinda pointless to me... There a reason it's done?
       
Post is unread #79 Aug 17, 2008, 7:54 pm
Go to the top of the page
Go to the bottom of the page

Kayle
Off the Edge of the Map
GroupAdministrators
Posts1,195
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, 8:42 pm   Last edited Aug 17, 2008, 8: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 >>