std::string <--> System::String*

[Marcus Kwok]

Hello, I am working on cleaning up some code that I inherited and was
wondering if there is anything wrong with my function. I am fairly
proficient in standard C++ but I am pretty new to the .NET managed C++.
It seems to work fine, but everyone knows that programs with errors can
still appear to "work fine"

I am working with VS .NET 2003; I am unable to upgrade to 2005 at this
time, so I cannot use the newer syntax or features.

This function takes a std::string and converts it to a System::String*.


Here is the original:

System::String*
MarshalStdToNetString(std::string& ss)
{
if (ss.empty())
return new System::String(S"");

System::IntPtr ptr(static_cast<System::IntPtr>(static_cast<void*>(const_cast<char*>(ss.c_str()))));

System::String* ret(System::Runtime::InteropServices::Marshal:trToStringAnsi(ptr));

return ret;
}


Here is my updated version:
// function name is just shorthand for what I will eventually call it

System::String*
std2gc(const std::string& s)
{
return new System::String(s.c_str());
}


Have I oversimplified? Or is all that casting business really needed?


Also, going in the other direction, is this the proper way to convert a
managed System::String* into a std::string? I haven't started trying to
change this one yet:

void
MarshalNetToStdString(System::String* s, std::string& os)
{
using System::IntPtr;
using System::Runtime::InteropServices::Marshal;

const char* chars = (const char*)(Marshal::StringToHGlobalAnsi(s)).ToPointer();
os = chars;
Marshal::FreeHGlobal(IntPtr((void*)chars));
}

 

[Jochen Kalmbach [MVP]]

Hi Marcus!

Here is my updated version:
// function name is just shorthand for what I will eventually call it

System::String*
std2gc(const std::string& s)
{
return new System::String(s.c_str());
}


Have I oversimplified?

No. That look´s really good!

Also, going in the other direction, is this the proper way to convert a
managed System::String* into a std::string? I haven't started trying to
change this one yet:

void
MarshalNetToStdString(System::String* s, std::string& os)
{
using System::IntPtr;
using System::Runtime::InteropServices::Marshal;

const char* chars = (const char*)(Marshal::StringToHGlobalAnsi(s)).ToPointer();
os = chars;
Marshal::FreeHGlobal(IntPtr((void*)chars));
}

It looks ok...

--
Greetings
Jochen

My blog about Win32 and .NET
http://blog.kalmbachnet.de/

 

[Marcus Kwok]

Hi Marcus!


No. That look?s really good!

OK, thanks!

It looks ok...

Thanks, after looking around some more I found almost identical code in
MSDN, so I think this is the "right" way to do it, though maybe I'll
change it to return the std::string instead of passing an out-parameter
to the function.

 

[Bronek Kozicki]

This function takes a std::string and converts it to a
System::String*.

you should also think about std::wstring conversions; potentially these
could be faster, as CLI String is unicode one which means there would't
be conversion in String constructor and marshaller, and you would't lose
data (anything beyond ASCII)


B.

 

[Marcus Kwok]

you should also think about std::wstring conversions; potentially these
could be faster, as CLI String is unicode one which means there would't
be conversion in String constructor and marshaller, and you would't lose
data (anything beyond ASCII)

Thanks for the idea. However, this code is in a GUI that just passes
the strings to a backend calculation engine, so conversion speed is not
an issue. Also, all of our data is plain ASCII so I don't need to
support Unicode.

 

[Marcus Kwok]

Also, going in the other direction, is this the proper way to convert a
managed System::String* into a std::string? I haven't started trying to
change this one yet:

void
MarshalNetToStdString(System::String* s, std::string& os)
{
using System::IntPtr;
using System::Runtime::InteropServices::Marshal;

const char* chars = (const char*)(Marshal::StringToHGlobalAnsi(s)).ToPointer();
os = chars;
Marshal::FreeHGlobal(IntPtr((void*)chars));
}

Here is my new implementation. Personally I feel this is a little
cleaner since it eliminates a few casts and the interface is more
consistent with the other (snipped) function, but it is essentially the
same:

std::string
gc2std(System::String* s)
{
using System::IntPtr;
using System::Runtime::InteropServices::Marshal;

IntPtr ip = Marshal::StringToHGlobalAnsi(s);
std::string ss = static_cast<const char*>(ip.ToPointer());
Marshal::FreeHGlobal(ip);
return ss;
}

 

[Bronek Kozicki]

std::string
gc2std(System::String* s)
{
using System::IntPtr;
using System::Runtime::InteropServices::Marshal;

IntPtr ip = Marshal::StringToHGlobalAnsi(s);
std::string ss = static_cast<const char*>(ip.ToPointer());
Marshal::FreeHGlobal(ip);
return ss;
}

nice, however I see slight exception safete problem here : std::string
constructor might throw std::bad_alloc (eg. due to high memory
fragmentation and/or very long string being constructed) and in such
situation Marshal::FreeHGlobal will not be called.


B.

 

[Marcus Kwok]

nice, however I see slight exception safete problem here : std::string
constructor might throw std::bad_alloc (eg. due to high memory
fragmentation and/or very long string being constructed) and in such
situation Marshal::FreeHGlobal will not be called.

Ahh, yes, good point. This is exactly the kind of feedback I was
looking for I need to work on improving my exception-safety
abilities.

So, is this exception-safe? Although I vaguely recall reading somewhere
that duplicating the clean-up code like this is considered bad style. I
guess this is why the original version used the output parameter
(reference) to save the string, but I wonder if the assignment operator
in the original could cause the same issue.


std::string
gc2std(System::String* s)
{
using System::IntPtr;
using System::Runtime::InteropServices::Marshal;

IntPtr ip = Marshal::StringToHGlobalAnsi(s);

std::string ss;
try {
ss = static_cast<const char*>(ip.ToPointer());
}
catch (std::bad_alloc&) {
Marshal::FreeHGlobal(ip);
throw;
}

Marshal::FreeHGlobal(ip);

return ss;
}

 

[Jochen Kalmbach [MVP]]

Hi Marcus!

try {
ss = static_cast<const char*>(ip.ToPointer());
}
catch (std::bad_alloc&) {
Marshal::FreeHGlobal(ip);
throw;
}

Marshal::FreeHGlobal(ip);

I suggest to use "try-finally" and remove the "using System::IntPtr;":

String *s = S"abc";
using System::Runtime::InteropServices::Marshal;
IntPtr ip = Marshal::StringToHGlobalAnsi(s);
std::string ss;
try
{
ss = static_cast<const char*>(ip.ToPointer());
}
__finally
{
Marshal::FreeHGlobal(ip);
}
return ss;



--
Greetings
Jochen

My blog about Win32 and .NET
http://blog.kalmbachnet.de/

 

[Jochen Kalmbach [MVP]]

Better would be:

void Conv(System::String *s, std::string &ss)
{
using System::Runtime::InteropServices::Marshal;
IntPtr ip = Marshal::StringToHGlobalAnsi(s);
ss.clear();
try
{
ss = static_cast<const char*>(ip.ToPointer());
}
__finally
{
Marshal::FreeHGlobal(ip);
}
}

So the memory will only be allocated once...

--
Greetings
Jochen

My blog about Win32 and .NET
http://blog.kalmbachnet.de/

 

[Jochen Kalmbach [MVP]]

Better would be:

Ok, last try:

void Conv(System::String *s, std::string &ss)
{
ss.clear();
if (s == 0) return;
using System::Runtime::InteropServices::Marshal;
IntPtr ip = Marshal::StringToHGlobalAnsi(s);
try
{
ss = static_cast<const char*>(ip.ToPointer());
}
__finally
{
Marshal::FreeHGlobal(ip);
}
}



--
Greetings
Jochen

My blog about Win32 and .NET
http://blog.kalmbachnet.de/

 

[Jeff F]

Knowing nothing of managed / CLR would the following not suffice

Ok, last try:

void Conv(System::String *s, std::string &ss)
{
ss.clear();
if (s == 0) return;
using System::Runtime::InteropServices::Marshal;

struct SafeStringAccessor
{
IntPtr mIp;

SafePtr( System::String* s ):mIp(Marshal::StringToHGlobalAnsi(s)){}
~SafePtr(){ mIp(Marshal::FreeHGlobal(mIp); }

const char* c_str()const
{
return static_cast<const char*>(mIp.ToPointer());
}
}

ss = SafeStringAccessor(s).c_str();

}

Of course with the idea of parameterizing this and/or using
boost::shared_ptr with an appropriate deleter.

Thanks, Jeff Flinn

 

[Marcus Kwok]

Knowing nothing of managed / CLR would the following not suffice


struct SafeStringAccessor
{
IntPtr mIp;

SafePtr( System::String* s ):mIp(Marshal::StringToHGlobalAnsi(s)){}
~SafePtr(){ mIp(Marshal::FreeHGlobal(mIp); }

const char* c_str()const
{
return static_cast<const char*>(mIp.ToPointer());
}
}

ss = SafeStringAccessor(s).c_str();

Of course with the idea of parameterizing this and/or using
boost::shared_ptr with an appropriate deleter.

Yes, I also do prefer the RAII idiom over explicit try/catch blocks, and
I have been doing mainly ISO C++ and not very much .NET stuff so I
didn't know about the __finally keyword.


I had to modify some things to get it to compile under /clr:

(I do not like to use using directives, and my compiler is telling me
that I cannot add a using declaration for Marshal, which is why I
fully-qualify the name, and also apparently you can't use the 'const'
qualifier on managed types)


__gc class SafeStringAccessor {
public:
SafeStringAccessor(System::String* s) : ip_(System::Runtime::InteropServices::Marshal::StringToHGlobalAnsi(s)) { }

~SafeStringAccessor() { System::Runtime::InteropServices::Marshal::FreeHGlobal(ip_); }

const char* c_str()
{
return static_cast<const char*>(ip_.ToPointer());
}
private:
System::IntPtr ip_;
};


std::string ss = (new SafeStringAccessor(s))->c_str();


But now this is starting to look a little bit messier than the
stand-alone functions.

 

[Jochen Kalmbach [MVP]]

Hi Marcus!

__gc class SafeStringAccessor {

You shouldn´t declare it as managed type... this might lead to leaks!
Because you have a undeterministic call of the finalizer (destructor) in
the managed world!!


The following should be correct:

struct Conv
{
char *szAnsi;
Conv(System::String* s)
:
szAnsi(static_cast<char*>(System::Runtime::InteropServices::Marshal::StringToHGlobalAnsi(s).ToPointer()))
{}
~Conv()
{

System::Runtime::InteropServices::Marshal::FreeHGlobal(IntPtr(szAnsi));
}
const char* c_str() const
{
return szAnsi;
}
};


int _tmain()
{
String *s = S"abc";
std::string ss = Conv(s).c_str();
return 0;
}

public:
SafeStringAccessor(System::String* s) : ip_(System::Runtime::InteropServices::Marshal::StringToHGlobalAnsi(s)) { }

~SafeStringAccessor() { System::Runtime::InteropServices::Marshal::FreeHGlobal(ip_); }

const char* c_str()
{
return static_cast<const char*>(ip_.ToPointer());
}
private:
System::IntPtr ip_;
};


std::string ss = (new SafeStringAccessor(s))->c_str();


But now this is starting to look a little bit messier than the
stand-alone functions.

--
Greetings
Jochen

My blog about Win32 and .NET
http://blog.kalmbachnet.de/

 

[Jeff F]

Hi Marcus!

You shouldn´t declare it as managed type... this might lead to leaks!
Because you have a undeterministic call of the finalizer (destructor)
in the managed world!!


The following should be correct:

struct Conv
{
char *szAnsi;
Conv(System::String* s)
:
szAnsi(static_cast<char*>(System::Runtime::InteropServices::Marshal::StringToHGlobalAnsi(s).ToPointer()))
{}
~Conv()
{

System::Runtime::InteropServices::Marshal::FreeHGlobal(IntPtr(szAnsi));
}
const char* c_str() const
{
return szAnsi;
}
};

and to further simplify:


std::string StdStringFrom( System::String* s)
{
return Conv(s).c_str();
}

int _tmain()
{
String *s = S"abc";

std::string ss = StdStringFrom(s);

return 0;
}

Jeff Flinn

 

[Marcus Kwok]

and to further simplify:


std::string StdStringFrom( System::String* s)
{
return Conv(s).c_str();
}


std::string ss = StdStringFrom(s);

Thank you everybody. I think this version is the cleanest (except I
made szAnsi private in the Conv struct/class).

 

[Marcus Kwok]

Thank you everybody. I think this version is the cleanest (except I
made szAnsi private in the Conv struct/class).

So, to summarize, here is the final implementation I have decided on,
barring any other issues with this code. I especially like the symmetry
between the two Marshal* functions.


#ifndef MARSHAL_STRINGS_H
#define MARSHAL_STRINGS_H

#include <string>

#using <mscorlib.dll>

class Conv {
public:
Conv(System::String* s)
: cp(static_cast<char*>(System::Runtime::InteropServices::Marshal::StringToHGlobalAnsi(s).ToPointer()))
{ }

~Conv()
{
System::Runtime::InteropServices::Marshal::FreeHGlobal(static_cast<System::IntPtr>(cp));
}

const char* c_str() const
{
return cp;
}

private:
char* cp;
};



// declared inline so I can keep the implementation in a header file
// without getting linker errors
inline
std::string
MarshalNetToStdString(System::String* s)
{
return std::string(Conv(s).c_str());
}



// declared inline so I can keep the implementation in a header file
// without getting linker errors
inline
System::String*
MarshalStdToNetString(const std::string& s)
{
return new System::String(s.c_str());
}

#endif

 

[Jochen Kalmbach [MVP]]

Hi Marcus!

Why not use:

struct Conv
{
char *szAnsi;
Conv(System::String* s)
:
szAnsi(static_cast<char*>(System::Runtime::InteropServices::Marshal::StringToHGlobalAnsi(s).ToPointer()))
{}
~Conv()
{
System::Runtime::InteropServices::Marshal::FreeHGlobal(IntPtr(szAnsi));
}
operator LPCSTR() const
{
return szAnsi;
}
};


Then you can simply write:


std::string ss = Conv(s);

--
Greetings
Jochen

My blog about Win32 and .NET
http://blog.kalmbachnet.de/

 

[Marcus Kwok]

Hi Marcus!

Why not use:

struct Conv
{
char *szAnsi;
Conv(System::String* s)
:
szAnsi(static_cast<char*>(System::Runtime::InteropServices::Marshal::StringToHGlobalAnsi(s).ToPointer()))
{}
~Conv()
{
System::Runtime::InteropServices::Marshal::FreeHGlobal(IntPtr(szAnsi));
}
operator LPCSTR() const
{
return szAnsi;
}
};


Then you can simply write:

std::string ss = Conv(s);

Nice, now this eliminates the need for a second wrapper function


class MarshalNetToStdString {
public:
MarshalNetToStdString(System::String* s)
: cp(static_cast<char*>(System::Runtime::InteropServices::Marshal::StringToHGlobalAnsi(s).ToPointer()))
{ }

~MarshalNetToStdString()
{
System::Runtime::InteropServices::Marshal::FreeHGlobal(static_cast<System::IntPtr>(cp));
}

operator const char*() const
{
return cp;
}

private:
char* cp;

MarshalNetToStdString(const MarshalNetToStdString&);
MarshalNetToStdString& operator=(const MarshalNetToStdString&);
};


However, thinking about it, I was reminded of the "rule of 3" (in
essence, it says that if you provide any of {destructor, copy
constructor, assignment operator} then you probably need to provide all
three of them). I felt that it wouldn't really make much sense to be
copying these objects around, so I made the copy constructor and
assignment operator private to prevent the compiler from generating
default ones that will only perform shallow copies. The default
constructor does not get auto-generated since I have a user-defined
constructor, so I don't have to worry about that one.


Please see if the following train of thought sounds logical:
I only intend to use this class in the context of

System::String* s = new System::String("...");
std::string ss = MarshalNetToStdString(s);

In other words, create a temporary object (which gets destroyed at the
end of the statement) to do the conversion. In order to avoid the
overhead of creating/destroying an object every time, I thought about
making static member functions to do the conversion, but then I realized
that this would make the memory management much more complex, so I
decided against it. Plus, this class seems pretty lightweight so I
don't think the overhead is too great compared to the safety it
provides.

 

[Jeff F]

Hi Marcus!

Why not use:

struct Conv
{
char *szAnsi;
Conv(System::String* s)
:
szAnsi(static_cast<char*>(System::Runtime::InteropServices::Marshal::StringToHGlobalAnsi(s).ToPointer()))
{}
~Conv()
{
System::Runtime::InteropServices::Marshal::FreeHGlobal(IntPtr(szAnsi));
}
operator LPCSTR() const
{
return szAnsi;
}
};


Then you can simply write:


std::string ss = Conv(s);

This approach was discarded(by me) primarily to avoid the nasty surpises
that go along with implicit conversions, which are the same reasons for
std::string::c_str().

Jeff Flinn