I added a patch to the Code Repository for std::string OLC utilities for RaM.

http://www.mudbytes.net/index.php?a=file...

This patch adds std::string versions of many utility functions and is a precursor to being able to use OLC on data structures that have std::string members. There are no data structure changes in this patch, except for the addition of a single pointer in descriptor_data (see below).

Realistically this should be peer reviewed by someone else, so I am asking for any and all feedback/review of my patch.

Summary of changes:
merc.h - added pString_std to descriptor_data structure to support editing std::string's via OLC.

olc.h / olc_utils.c -
* Modified functions to all check both the pString and pString_std pointers to decide if the currently edited string is a c-style string or std::string.
* Added utility functions for string_append(), string_linedel(), and string_lineadd() with std::string arguments.
* Broke up format_string() - all the logic is now in format_string_const(), which takes a 'const char *' arg and always allocates a new string for its return value. Changed format_string() to call format_string_const() and perform some manual memory management. Also added a version of format_string() that takes a std::string arg.
* Changed return type and arg types of numelineas() and getline() functions to be 'const char *' from 'char *'. A little safer this way and now these functions can be used with std::string's c_str() pointer.

Some comments…

- Why "std::string& pString"? It's not a pointer…
- When possible string refs should be const.
- You don't need to give functions the name "const", C++ automatically picks the right one if available and if possible.
- Is the replace function supposed to replace all occurrences or just one?
- A more descriptive name than "pString" or "pString_std" would be kind of nice…
- Is it really appropriate for format_string to free the string that was passed in??
- Shouldn't format_string be returning the result instead of editing the string that was passed in?
- Why was a blank line added at line 416 (of the diff, it's about line 290 in merc.h)
- I probably wouldn't leave in the old commented struct members, but that's not really important.

Generally it looks good.

David, Thanks for taking the time to give me feedback.


For one, to make it clear that a NULL pointer can't be passed to the function, and also that there shouldn't be any manual memory management (as seems to occur in some of the functions that my utilities are based on.O


Of course, it's not possible when the strings are being operated on inside the function.

Thanks. Originally that's what I did, but I wasn't 100% confident that the compiler would do what I wanted it to. The behavior of the '_const' function and the 'plain' function are a little different, so I wanted to make that clear.


Most of what you are pointing out are issues from the original code. My goal was to provide compatibility, so I didn't clean up any function names, behaviors, etc, beyond a few items. I held off on further cleanup in the interest of keeping the changes to a palatable degree of size. The original replace function only replaces the first occurrence, so that's what my version does as well.


I hand edited the diff and must have missed something. Also, I forgot to use the ignore-whitespace switch 8)

Thanks!

Actually, a reference *IS* a pointer, as far as the compiler is concerned. It's just that it shouldn't ever be possible to have the compiler generate a NULL reference or a reference whose actual data is mismatched, and that the compiler also automatically de-references it for you. That's why the reference token is so similar to the C "address of" operator.

I finally got the first draft of the ban conversion in place, although I'm looking at tossing a few string utilities in and trying to get rid of all the char *'s used inside the functions as well.

I wish we could rename the C++ files to .cpp, but that would only be useful during the transition… once everything is C++ enabled, we'd want to change them back to .c again I bet. :)

Man! The ban conversion was the simplest one! ;)

I would actually like to see the .c files in the Fire branch renamed to .cpp. If for no other reason, than so my IDE can understand and format the syntax properly. I don't see why we would want to go back later? This should be very easy for you to do, since you are the master of the svn repos.

Heh, your IDE should use the file's magic type (man (5) magic), instead of relying on the file extension like a 1980's DOS program. :)

However, unlike cvs, svn does allow you to rename files without losing your entire change history… so it's an idea.

Most IDE's have some form of an autodetect (based on file type) but normally you can select a different highlighting option. Just look around for syntax highlighting! Should be there somewhere.

I taken exception to that. My IDE (emacs) is actually a 1970s PDP-10 program. So be nice to it! It's old!

Hmm, I can't remember what happens if you construct a std::string from NULL. It might just be an empty string. When you have a std::string&, the compiler will construct a new std::string if whatever you're passing in isn't already a std::string – assuming of course that an appropriate constructor exists.

Still, I really don't like sticking "p" in front of things, but that's probably just preference to some extent. :smile:


This is a very nice feature of C++. There's a well-defined process for picking the appropriate overloaded function. For example,

void f(int i) { … }
void f(double d) { … }

If you call f with an int, it will pick the first one; if you call it with a double it will pick the second one. If you call it with something else that can only be converted to int (if such a thing exists), it picks the first one. If you call f with something that can be converted to either one, the compiler should issue an error about not being able to figure out which function is correct.

Picking the appropriate "constness function" works similarly. Really, the best thing to do is to just try it out a bit with very simple functions that print out their name and argument types to see how the matching works.


Fair enough! :smile:


Hmm, generally I strongly discourage hand-editing diffs, because you might introduce/remove things that really shouldn't be modified. :smile: Also, I tend to avoid making non-helpful whitespace changes, because when looking at changes over time it can somewhat clutter up the change list. A single line doesn't really matter that much, though…

So what does gcc rely on?
*kof*

-x? :P

Well, gcc 4.0.1 doesn't let you get away with any shenanigans here. I just wrote a small test program.
* Initializing a std::string to NULL throws an exception.
* Passing an otherwise 'constructible' type to a function that expects a reference errors on compilation.
* Dereferencing a NULL pointer gives a Bus error. This is a hard one to debug, I know from experience.

I think (e(m(a(c(s))))) should be smart enough to figure it out… Doesn't it load gcc/g++ as a module to hold in-memory, just in case you wanted to compile that mail message before you post it to USENET via the IRC gateway that's built in?

*bam!*

There, I have appeased the elder gods of vi and ed.

Maybe we're not talking about the same thing?

test.cpp:

#include <iostream>
#include <string>

void foo(const std::string& s)
{
    std::cout << s << std::endl;
}

int main(char ** argv, int argc)
{
    foo("c-style string");
    foo(std::string("std::string"));
}

$g++ test.cpp
./a.out
c-style string
std::string

Maybe you didn't make the reference const. That's a good thing actually – it would be Very Bad to be messing around with references to temporaries like that.

Yes I was talking about volatile references, not const references. I think it's pretty self-evident why the compiler allows 'temporary' objects to be passed as const reference but not 'volatile' references. Interesting that the compiler is sophisticated enough to distinguish these.

Oh, the compiler has all kinds of rules for dealing with constness and doing the right thing based on constness – you have const methods for example that cannot modify anything in the object, or call any other methods on the object that aren't also const. And of course you can't call a non-const method on a const object.

The new C++ standard introduces "compile-time const" functions that can only call other const functions, and that have a fairly reduced set of operations – the compiler will resolve the function call at compile-time and treat it like a normal const int (or whatever) for all intents and purposes. Pretty nifty because it lets you write constants with math in them, which you previously had to do with macros (which could get kind of icky).