Gammon Forum : SMAUG : SMAUG coding : Converting from GCC to G++

Register forum user name Search FAQ

Gammon Forum

Entire forum

SMAUG

SMAUG coding

Converting from GCC to G++

Converting from GCC to G++

It is now over 60 days since the last post. This thread is closed. Refresh page

Pages: 1 2

Posted by Atrox USA (18 posts) bio

Date Fri 19 Mar 2010 01:35 AM (UTC)

Message

So, I recently started converting my heavily modified SmaugFUSS 1.6 from compiling with GCC to G++. I used the most recent SmaugFUSS 1.9 as a reference, and FINALLY got it all compiling and linking properly today. Now, the problem is... the interpreter is crashing when trying to allocate memory for a temporary pointer to convert input from a normal character pointer to a constant.

Just as in the regular SmaugFUSS 1.9, I'm using this function for that:

(225)  void interpret( CHAR_DATA * ch, const char* argument)
(226)  {
(227)      char *temp = strdup(argument);
(228)      interpret(ch, temp);
(229)      free(temp);
(230)  }

In GDB, this is what I get:

#0  0xb7b6086c in _int_malloc () from /lib/libc.so.6
#1  0xb7b6250f in calloc () from /lib/libc.so.6
#2  0x080f47a0 in str_dup (str=0x86531f8 "who") at db.c:3447
#3  0x0812d775 in interpret (ch=0x83a73e8, argument=0x86531f8 "who") at interp.c:227
#4  0x0812d789 in interpret (ch=0x83a73e8, argument=0x86531e8 "who") at interp.c:228
#5  0x0812d789 in interpret (ch=0x83a73e8, argument=0x86531d8 "who") at interp.c:228

Posted by Nick Gammon Australia (22,975 posts) bio Forum Administrator

Date Reply #1 on Fri 19 Mar 2010 02:28 AM (UTC)

Message

Atrox said:

Just as in the regular SmaugFUSS 1.9, I'm using this function for that:

(225)  void interpret( CHAR_DATA * ch, const char* argument)
(226)  {
(227)      char *temp = strdup(argument);
(228)      interpret(ch, temp);
(229)      free(temp);
(230)  }

The function interpret is calling itself. Is that intentional?

- Nick Gammon

www.gammon.com.au, www.mushclient.com

Posted by Atrox USA (18 posts) bio

Date

Reply #2 on Fri 19 Mar 2010 02:34 AM (UTC)

Amended on Fri 19 Mar 2010 02:37 AM (UTC) by Atrox

Message

It's an overloaded function, the first one that's called is through the main loop using a constant character pointer for arg2, and then it passes it to another interpret function that takes a regular character pointer.


// New overloaded function to accept const char *'s
void interpret( CHAR_DATA * ch, const char* argument)
{
    char *temp = strdup(argument);
    interpret(ch, temp);
    free(temp);
}

// Original Smaug implementation
void interpret( CHAR_DATA * ch, char *argument )
{

Posted by

Hokken USA (18 posts)

Date

Reply #3 on Fri 19 Mar 2010 03:04 AM (UTC)

Message

you really can't overload a function based on the const-ness of a pointed to type in C++ - it's too ambiguous. that's why your first interpret() function is calling itself recursively until you either overflow your stack or run out of heap. why are you overloading the function in the first place if all you are doing is calling the original one? if you really need to have two different functions then you have to either name them differently or use unambiguously different type signatures.

Posted by Nick Gammon Australia (22,975 posts) bio Forum Administrator

Date

Reply #4 on Fri 19 Mar 2010 03:12 AM (UTC)

Amended on Fri 19 Mar 2010 03:21 AM (UTC) by Nick Gammon

Message

Well I thought you couldn't have function declarations that only differ by const-ness, but the g++ compiler seems to accept that.

This reproduces your problem:


#include <string.h>
#include <stdlib.h>
#include <stdio.h>

// New overloaded function to accept const char *'s
void interpret(char * ch, const char* argument)
  {
  char *temp = strdup(argument);
  interpret(ch, temp);
  free(temp);
  }

// Original Smaug implementation
void interpret( char * ch, char *argument )
  {
  printf ("argument is %s\n", argument);
  }

int main (void)
  {
  interpret ("nick", "who");
  return 0;
  }

Running it:


g++ -g3 test.cpp

$ ./a.out
Segmentation fault

gdb ./a.out
(gdb) run


#0  0xb7e8494c in ?? () from /lib/tls/i686/cmov/libc.so.6
#1  0xb7e868c5 in malloc () from /lib/tls/i686/cmov/libc.so.6
#2  0xb7e8a060 in strdup () from /lib/tls/i686/cmov/libc.so.6
#3  0x08048540 in interpret (ch=0x8048670 "nick", argument=0xa021b98 "who")
    at test.cpp:9
#4  0x08048555 in interpret (ch=0x8048670 "nick", argument=0xa021b88 "who")
    at test.cpp:10
#5  0x08048555 in interpret (ch=0x8048670 "nick", argument=0xa021b78 "who")
    at test.cpp:10
#6  0x08048555 in interpret (ch=0x8048670 "nick", argument=0xa021b68 "who")
    at test.cpp:10
#7  0x08048555 in interpret (ch=0x8048670 "nick", argument=0xa021b58 "who")
    at test.cpp:10
#8  0x08048555 in interpret (ch=0x8048670 "nick", argument=0xa021b48 "who")
    at test.cpp:10

Clearly it is recursing, as your own results show:


#0  0xb7b6086c in _int_malloc () from /lib/libc.so.6
#1  0xb7b6250f in calloc () from /lib/libc.so.6
#2  0x080f47a0 in str_dup (str=0x86531f8 "who") at db.c:3447
#3  0x0812d775 in interpret (ch=0x83a73e8, argument=0x86531f8 "who") at interp.c:227
#4  0x0812d789 in interpret (ch=0x83a73e8, argument=0x86531e8 "who") at interp.c:228
#5  0x0812d789 in interpret (ch=0x83a73e8, argument=0x86531d8 "who") at interp.c:228

However the solution is simple enough, forward-declare the function:


#include <string.h>
#include <stdlib.h>
#include <stdio.h>

void interpret( char * ch, char *argument );

// New overloaded function to accept const char *'s
void interpret(char * ch, const char* argument)
  {
  char *temp = strdup(argument);
  interpret(ch, temp);
  free(temp);
  }

// Original Smaug implementation
void interpret( char * ch, char *argument )
  {
  printf ("argument is %s\n", argument);
  }

int main (void)
  {
  interpret ("nick", "who");
  return 0;
  }

Now that runs OK. However I am inclined to agree with Hokken that it is a bit dangerous.

- Nick Gammon

www.gammon.com.au, www.mushclient.com

Posted by

Twisol USA (2,257 posts)

Date

Reply #5 on Fri 19 Mar 2010 03:23 AM (UTC)

Message

As I see it, there are two possibilities here.

(1) The function doesn't modify the string. If it does, the mutation is an unimportant side-effect.

(2) The function does modify the string, and the mutated string is part of the result set (C#'s "out" parameter keyword says it better).

If 1, you should just modify the original function to take a const char*. A char* can be casted to a const char* implicitly, so this is no trouble to the user. Then, if the function modifies the string (but it's not an actual "result" of the function, just a mere side-effect)) just put your strdup() code into the function itself.

If 2, you probably shouldn't be wrapping it with strdup, because you're trimming data that might be needed elsewhere. Use strdup in those locations rather than in the function.

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

Posted by

Atrox USA (18 posts)

Date

Reply #6 on Fri 19 Mar 2010 03:23 AM (UTC)

Message

I still don't get why it works just fine in SmaugFUSS 1.9, but not here. It's already forward declared, in mud.h, but I changed the original Smaug function to interpret2, and prototyped it above the new function, and now it works fine. Except that in converting my code to compile with G++ I have broken my dynamic linker, so now I get to come home after work tomorrow and try to fix that. :(

Thanks a lot for the help.

Posted by

David Haley USA (3,881 posts)

Date

Reply #7 on Fri 19 Mar 2010 05:20 AM (UTC)

Message

Quote:
you really can't overload a function based on the const-ness of a pointed to type in C++ - it's too ambiguous.

Sure you can; you just have to be sure that both prototypes are available whenever you use it.

Quote:
why are you overloading the function in the first place if all you are doing is calling the original one?

Because it's very convenient to be able to just call 'interpret' without worrying about the constness. The real implementation expects a modifiable string. But you occasionally want to interpret a constant string. So, you wrap the real implementation with a version that creates a modifiable string before disposing of it.

Twisol, your cases don't actually work in general. You assume that if it modifies it, those modifications are necessarily uninteresting. I'm not sure why you're making that assumption. It's perfectly reasonable to care about the modifications in some cases, and not care in others. If you have a const char* and actually need modifications, then you do the str dup yourself. But there's no point creating pain if you don't need the modifications most of the time.

That said, this is probably legacy, because the do_fun's have all been converted to take const char* arguments. That was the original reason behind all of this, IIRC -- if interp was const char*, it would have to be str_dup'ing internally anyhow because of what the commands were doing.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org

Posted by

Twisol USA (2,257 posts)

Date

Reply #8 on Fri 19 Mar 2010 05:29 AM (UTC)

Amended on Fri 19 Mar 2010 05:34 AM (UTC) by Twisol

Message

David Haley said:
Twisol, your cases don't actually work in general. You assume that if it modifies it, those modifications are necessarily uninteresting. I'm not sure why you're making that assumption. It's perfectly reasonable to care about the modifications in some cases, and not care in others.

I only assume that in case 1, really. Case 2 assumes that the modifications are interesting. Basically, just use const char* if there are no modifications, or the modifications are internal; use char* if there are modifications and they're "interesting". But in neither case should you need to create an overloaded function just to strdup, I'd think.

David Haley said:
If you have a const char* and actually need modifications, then you do the str dup yourself.

==

Twisol said:
If 2, you probably shouldn't be wrapping it with strdup, because you're trimming data that might be needed elsewhere. Use strdup in those locations rather than in the function.

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

Posted by

Nick Gammon Australia (22,975 posts)

bio Forum Administrator

Date

Reply #9 on Fri 19 Mar 2010 05:41 AM (UTC)

Message

Atrox said:

I still don't get why it works just fine in SmaugFUSS 1.9, but not here. It's already forward declared, in mud.h, ...

You didn't forward declare the non-const one, or you wouldn't have the crash.

- Nick Gammon

www.gammon.com.au, www.mushclient.com

Posted by

David Haley USA (3,881 posts)

Date

Reply #10 on Fri 19 Mar 2010 05:49 AM (UTC)

Message

Twisol, your cases are:

1. No modifications, or modifications uninteresting
2. Modifications are always interesting

You're leaving out the case I pointed out: modifications are only sometimes interesting, and you don't want to have to go through any hassle when you don't care about them.

Using your suggestions, if you force the function to take a char* on the assumption that the modifications are always interesting, then callers who don't care must do extra work. A simple overloaded wrapper fixes this completely.

You also incorrectly equate my suggestion and your case 2. My suggestion is that you overload the function so that it's easy to not care, but in those cases where you do care, and all you have is a const char*, only then must you do any extra work.

In other words, my suggestion minimizes work necessary in all cases.

There are four cases:
1. you care and you have a char*, and everything works
2. you don't care and you have a char*, and everything works
3. you care and you have a const char*, you need to do some work
4. you don't care and you have a const char*, and everything works

Your suggestion, however, needlessly forces extra work in case #4.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org

Posted by

Twisol USA (2,257 posts)

Date

Reply #11 on Fri 19 Mar 2010 05:59 AM (UTC)

Message

David Haley said:

Twisol, your cases are:

1. No modifications, or modifications uninteresting
2. Modifications are always interesting

You're leaving out the case I pointed out: modifications are only sometimes interesting, and you don't want to have to go through any hassle when you don't care about them.

Using your suggestions, if you force the function to take a char* on the assumption that the modifications are always interesting, then callers who don't care must do extra work. A simple overloaded wrapper fixes this completely.

You also incorrectly equate my suggestion and your case 2. My suggestion is that you overload the function so that it's easy to not care, but in those cases where you do care, and all you have is a const char*, only then must you do any extra work.

In other words, my suggestion minimizes work necessary in all cases.

There are four cases:
1. you care and you have a char*, and everything works
2. you don't care and you have a char*, and everything works
3. you care and you have a const char*, you need to do some work
4. you don't care and you have a const char*, and everything works

Your suggestion, however, needlessly forces extra work in case #4.

My definition of "interesting" in this case is more like "externally visible". If it modifies the source string at all, it should be "interesting". Correct me if I'm wrong, but your case 2 leaves this fact out, meaning the source string might be modified and you might not know about it.

Your cases are in regards to the user; mine are more in regards to the function itself.

1. If the function modifies the string and it should be publically visible, use char*.
2. If the function modifies the string and it shouldn't be publically visible, use const char*, and use strdup() internally.
3. If the function doesn't modify the string, use const char*.

Looking at how the user would use them:

1a. If you want the modifications, everything works.
1b. If you don't want the modifications, strdup.
2. Everything works.
3. Everything works.

The only work the user has to do is in the case of 1a, where you have to prevent the function from modifying a string that you want to preserve. This is logical.

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

Posted by

David Haley USA (3,881 posts)

Date

Reply #12 on Fri 19 Mar 2010 06:09 AM (UTC)

Amended on Fri 19 Mar 2010 06:10 AM (UTC) by David Haley

Message

Function APIs are necessarily from the user's perspective. You don't design APIs from an implementer's perspective but from a user's, unless there are rather stringent implementation details to worry about, typically having to do with serious performance hits. Your proposed API creates needless work in some cases, while never reducing the amount of work. I'm not sure how this could possibly be a good thing. :-)

You continue to assume, by the way, that either changes should always or never be externally visible. You leave out the case where you sometimes care and sometimes don't.

The only possible argument against this, really, is that you need to be aware that the function can make some modifications that you might care about. This would need to be made clear in the API documentation.

In the end of the day, it is as simple as optimizing for the common case. If the common case is that you care about the modifications, then it might make sense to force extra work only to save the user from reading documentation. If the common case is that you don't care about the modifications, it is rather unambiguously better to save the user from needing to care about them. If the common case is split, then prefer the solution that creates the least amount of work for users.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org

Posted by Twisol USA (2,257 posts) bio

Date

Reply #13 on Fri 19 Mar 2010 06:25 AM (UTC)

Amended on Fri 19 Mar 2010 06:28 AM (UTC) by Twisol

Message

David Haley said:

Function APIs are necessarily from the user's perspective. You don't design APIs from an implementer's perspective but from a user's, unless there are rather stringent implementation details to worry about, typically having to do with serious performance hits. Your proposed API creates needless work in some cases, while never reducing the amount of work. I'm not sure how this could possibly be a good thing. :-)

You continue to assume, by the way, that either changes should always or never be externally visible. You leave out the case where you sometimes care and sometimes don't.

The only possible argument against this, really, is that you need to be aware that the function can make some modifications that you might care about. This would need to be made clear in the API documentation.

In the end of the day, it is as simple as optimizing for the common case. If the common case is that you care about the modifications, then it might make sense to force extra work only to save the user from reading documentation. If the common case is that you don't care about the modifications, it is rather unambiguously better to save the user from needing to care about them. If the common case is split, then prefer the solution that creates the least amount of work for users.

Well, I'm also trying to keep in mind the actual function involved here, too. And when you say you "sometimes" care, how does the function deal with that? My preferred approach in that case would be something like this:

void interpret( CHAR_DATA * ch, const char* argument, char* out)

And you set 'out' to NULL when you don't care. A "sometimes" case, when handled without the 'out' parameter above, is not explicit enough for me. But considering the function at hand, it didn't seem like going that far was required. Maybe I was wrong.

Also, you should know me well enough by now to know that I usually go too far (in the eyes of some) in the name of user-friendliness, i.e. PPI. ;)

'Soludra' on Achaea

Blog: http://jonathan.com/
GitHub: http://github.com/Twisol

Posted by

David Haley USA (3,881 posts)

Date

Reply #14 on Fri 19 Mar 2010 01:29 PM (UTC)

Message

The 'out' parameter is another way to deal with this, but it (as usual) depends on what exactly is happening here. Sometimes it's more convenient to have the input operated on directly for whatever reason. For instance, it might be a more complicated data structure than just a char*. Also, it means more stuff to keep track of on the caller's end when they want to get the modifications; now they have to make the call and prepare the target buffer for the modifications. Like I said, it really depends on what the common case is.

All I was trying to say here is that there are reasonable cases besides "always care" and "never care", and so it can make sense to have this form of overloading. (Sometimes, the implementations might even be different, and the overloaded version wouldn't wrap it with a duplication but do some other computation that doesn't involve the side effect.) So the answer isn't a clear-cut "make it either const or non const but not both". Even in the case of the nullable out parameter, we see that this is happening (although out parameters, especially for some types in C/C++, have a whole new slew of issues of their own...).

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org

The dates and times for posts above are shown in Universal Co-ordinated Time (UTC).

To show them in your local time you can join the forum, and then set the 'time correction' field in your profile to the number of hours difference between your location and UTC time.

59,818 views.

This is page 1, subject is 2 pages long: 1 2

It is now over 60 days since the last post. This thread is closed. Refresh page

Go to topic: Search the forum

Quick links: MUSHclient. MUSHclient help. Forum shortcuts. Posting templates. Lua modules. Lua documentation.

Information and images on this site are licensed under the Creative Commons Attribution 3.0 Australia License unless stated otherwise.

Comments to: Gammon Software support
Forum RSS feed ( https://gammon.com.au/rss/forum.xml )