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 >
#61 Jul 25, 2008 5:59 pm
Sorcerer
GroupMembers
Posts902
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.
#62 Jul 25, 2008 6:01 pm
Black Hand
GroupAdministrators
Posts3,696
JoinedJan 1, 2002
Gah, I edited my last post before you posted your reply
#63 Jul 25, 2008 6:04 pm
Sorcerer
GroupMembers
Posts902
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". 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.
#64 Jul 25, 2008 6:32 pm
Black Hand
GroupAdministrators
Posts3,696
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.
#65 Jul 25, 2008 6:54 pm
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*
#66 Jul 27, 2008 4:18 pm
Last edited Jul 27, 2008 4:20 pm by David Haley
Sorcerer
GroupMembers
Posts902
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.
#67 Jul 27, 2008 10:43 pm
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:
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
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
#68 Jul 27, 2008 10:53 pm
Sorcerer
GroupMembers
Posts902
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.
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
#69 Jul 27, 2008 11:59 pm
Black Hand
GroupAdministrators
Posts3,696
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.
#70 Jul 28, 2008 5:26 am
Magician
GroupMembers
Posts121
JoinedJun 30, 2006
I did notice this memory leak in the diff:
and this:
not a bug per-say, but smaug practices IIRC is to use DISPOSE not free. this might confuse some people.
this:
think it should just be argument, not "argument"
I don't see how this is actually smashing tildes.
this looks like trouble just waiting to happen
just a quick scan over 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
#71 Jul 28, 2008 8:02 am
Sorcerer
GroupMembers
Posts902
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.
#72 Jul 28, 2008 10:26 am
Last edited Jul 28, 2008 10:28 am by David Haley
Sorcerer
GroupMembers
Posts902
JoinedJan 29, 2007
Here is the diff to account for the problems Kiasyn found.
Thanks Kiasyn
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).
=== 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
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).
#73 Jul 29, 2008 7:20 pm
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.
#74 Aug 17, 2008 4:37 pm
Last edited Aug 17, 2008 4:37 pm by Kayle
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:
And I have one question. How does this do what smash_tilde is supposed to do exactly?
Smash_tilde used to look like:
I'm not seeing how they work the same. >.>
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. >.>
#75 Aug 17, 2008 4:50 pm
Black Hand
GroupAdministrators
Posts3,696
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.
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.
#76 Aug 17, 2008 4:53 pm
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*
#77 Aug 17, 2008 7:44 pm
Sorcerer
GroupMembers
Posts902
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.
#78 Aug 17, 2008 8:11 pm
Off the Edge of the Map
GroupAdministrators
Posts1,200
JoinedMar 21, 2006
That seems kinda pointless to me... There a reason it's done?
#79 Aug 17, 2008 9:54 pm
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.
#80 Aug 17, 2008 10:42 pm
Last edited Aug 17, 2008 10:42 pm by David Haley
Sorcerer
GroupMembers
Posts902
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. 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...