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.

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

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.

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.

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*

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.

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

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.

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.

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

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.

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

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).

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.

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. >.>

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.

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

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.

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

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.

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...