Archived

This topic is now archived and is closed to further replies.

kVandaele

std::string, classes and... pointer?

Recommended Posts

I''m using a date class in my game called cTimeAndDate, which keeps track of the date in m_Year, m_Month, m_Day, m_Hour, m_Minute, m_Second. I''ve got an instance of cTimeAndDate in cTotal called m_Now. What i now want to do is have a member function of cTotal return a nicely formatted string to print the date on my screen.
cTotal::cTotal(){
	m_Now = cTimeAndDate(3174, 3, 24, 11, 56); 
	// year, month, day, hour, minute

}

std::string cTotal::GetNowString(std::string s){
	s = "Interstellar Time: ";
	s += m_Now.GetYear() + ".";
	if(m_Now.GetMonth() < 10)
		s += ''0'';
	s += m_Now.GetMonth() + ".";
	if(m_Now.GetDay() < 10)
		s += ''0'';
	s += m_Now.GetDay() + " ";
	if(m_Now.GetHour() < 10)
		s += ''0'';
	s += m_Now.GetHour() + ":";
	if(m_Now.GetMinute() < 10)
		s += ''0'';
	s += m_Now.GetMinute() + ":";
	if(m_Now.GetSecond() < 10)
		s += ''0'';
	s += m_Now.GetSecond();
	return s;
}
as you can see i pass along an std::string variable because before (when i was trying to do it with char strings - ouch) it gave me a warning that i was returning a pointer to a temporary variable. Here''s what i put in my winmain:
char Text[1024];
std::string tempText;
sprintf(Text, App->m_Total.GetNowString(tempText).c_str());
App->m_Font.Print(Text, 536, 7);
Also, there''s no problem with m_Font, or m_Font.Print(). Here''s the output i expected to be printed on screen: "Interstellar Time: 3174.03.24 11:56:00" Here''s what i got: "Interstellar Time: 0e: 0x900.8x0" "Interstellar Time: 0e: 0x0012e9xx0" Looking at the second one my thoughts were "looks like a pointer address" but then i''ve never know of two x''s in an address, or a period "."... Also, if they''re adresses why is one 7 chars and the other 9? thanks

Share this post


Link to post
Share on other sites

Let''s get this straight. You''re passing your function an empty string.. which isn''t modified, because it isn''t passed by reference.. basically, the copy of it on the stack is just used as a temporary variable.

Your mistake, though, is passing the string to sprintf as the format string. Look up sprintf''s documentation, say, here.

Hope that helps.

.bas

[sPiKie] mmorpg isnt hard in vb

Share this post


Link to post
Share on other sites
A guess the Get* methods return integers? So you know that you add an integer to a character pointer (beeing just pointer arithmetic). The string at the new address is then added to s.

Share this post


Link to post
Share on other sites
He''s not using a character pointer, he''s using a std::string, which overloads the + and += operators appropriately.

The sprintf call looks right to me, but there has to be a better way to convert std::string to char *. Is there a reason you can''t make the mFont.Print() method use a string instead? (wtf are those values ''536'' and ''7'' anyway? constants please?)

Your function doesn''t modify the input string, as noted; s will get pointed at a new string internally. And then you don''t do anything with the return value. You''ll need to pass in by reference; and then the return value is only worth something if you want to chain calls somehow, which I doubt is the case here. Anyway, the output you''re seeing is probably garbage from the uninitialized string. There might be a lot of non-printing characters in there too :/

Oh, and FFS do the getNowString() calculations in the cTimeandDate class, and delegate there from cTotal. It''s cleaner OO and should actually be faster (because within cTimeandDate you have direct access to the m_Year etc.... even if you made them public, they''re more directly accessible in cTimeandDate).

Share this post


Link to post
Share on other sites

I''m sorry zahlman, but you''re incorrect. The prototype of the sprintf is:

int sprintf ( char * buffer, const char * format [ , argument , ...] );

The second argument is the format string.

I don''t think the OP should be using the sprintf () at all, or use only sprintf. For an example use of sprintf:

sprintf (Text, "Interstellar time: %d %d %d", &day, &month, &year);

In this case, however, he should change the sprintf to:

sprintf (Text, "%s", App->m_Total.GetNowString(tempText).c_str());

.bas

[sPiKie] mmorpg isnt hard in vb

Share this post


Link to post
Share on other sites
okay, sorry for the long time to reply, i pulled off an all-nighter and dozed off.

first reply:
Hmm... Right, guess i shouldn''t pass the variable. I don''t know where that came from, i guess i need more experience with pass by reference...

you may be right, bastardos. However, i simplified that for my posting (and my attempts at fixing it). Both of the following lines produce the same gibberish:

sprintf(Text, "Credits: %u\n%s", App->m_Player.GetCredits(), App->m_Total.GetNowString(tempText).c_str());
sprintf(Text, App->m_Total.GetNowString(tempText).c_str());

Perhaps my mistake (if it was wrong) was to assume that sprintf just copied the second arguement into the first arguement (after replacing all %x''s).

Also, thanks for the link but i have it already bookmarked... yay for google

VolkerG:
Actually, they return DWORDS to be precise, but yes. So... what you''re saying is that the integers aren''t converted by the std::string class? I''d find that rather strange considering std::cout does it... I''ll test that with a sprintf later, sounds like that might be it.

Zahlman:
the cFont class is basically a wrapper for the following DirectX call:
INT DrawText(
LPCSTR pString,
INT Count,
LPRECT pRect,
DWORD Format,
D3DCOLOR Color
);
As you can see, DrawText takes a Long Pointer to C STRing (... i think). So, even if i modified the cFont.Print class to take a std::string variable, at some point i''d have to convert anyway.
Also, sorry for that, 536 is the x value and 7 the y value of the position to draw (top right corner on 800x600).

>>And then you don''t do anything with the return value.

I get where passing the variable to it doesn''t do anything, but i do return s, and then doesn''t this line call the c_str() of the returned std::string?
App->m_Total.GetNowString(tempText).c_str()
= s.c_str() ?

>>Oh, and FFS do the getNowString() calculations in the cTimeandDate class, and delegate there from cTotal. It''s cleaner OO and should actually be faster (because within cTimeandDate you have direct access to the m_Year etc.... even if you made them public, they''re more directly accessible in cTimeandDate).

Thanks, will do.

Thanks, iamjoesname, but i already do...

bastardos:
true, the second variable of sprintf is usually (and meant to be) a format string, however in my console app i''ve done the following before:
sprintf(str1, "Hello!");
Since that doesn''t produce any errors, i''m assuming the following wouldn''t either:
str2 = "Hello!";
sprintf(str1, str2);
(... well, initialize str2 right and it''d work. Sorry, not much good with Cstrings.

Share this post


Link to post
Share on other sites
Haven't posted in ages and I'm ill, so go easy on the typos. Anyway, one serious question: Why are you mixing C and C++ like this? C-style output functions are notoriously frought with problems. std::string and std::stringstream exist so that you use them in place of the outdated, and dangerous, C functions.

Shouldn't be a call to sprintf() anywhere.

Edit: Typed up example, which is actually useless, but hopefully it demonstrates the point. Fingers crossed it'll compile.


#include <iostream>
#include <sstream>
#include <string>

using std::cout;
using std::endl;

int main()
{
std::string format;
std::stringstream sstream;

sstream << "Window has been drawn " << 1 << " times.\n";
format = sstream.str();
cout << format << endl;

return 0;
}


-hellz

[edited by - hellz on January 25, 2004 10:17:44 PM]

Share this post


Link to post
Share on other sites
To the OP: there is not conversion from an inter to a std::string object. But <strstream> or <sstream> (which is the prefered one, can't remember) could be what you want (works like any other C++ stream class).
To Zahlman: The rhs of the s+= is a character pointer, no std::string appearing here.

[EDIT]strstreams are not my friends ;-), hope everything is correct now.[/Edit]


[edited by - VolkerG on January 26, 2004 4:30:47 AM]

Share this post


Link to post
Share on other sites
>>Anyway, one serious question: Why are you mixing C and C++ like this?

... cuz i don''t know any better It''s not like i insist on using C (opposite in fact, else i''d be doing it all with char arrays and pointers).

sstream eh :D Let''s do some research!

GOT IT WORKING!!! Woohoo! stringstream''s pretty awesome. Good to know

Here''s what i did:
First, i added GetString to cTimeAndDate Zahlman said it''s faster (and cleaner). Besides, i agree, the function apparently just got ''lost'' when moving it from console to win32.

Next, i added stringstream to my code:

std::string cTimeAndDate::GetString(){
std::stringstream ss;
ss << "Interstellar Time: " << m_Year << ".";
if(m_Month < 10)
ss << ''0'';
ss << m_Month << ".";
if(m_Day < 10)
ss << ''0'';
ss << m_Day << " ";
if(m_Hour < 10)
ss << ''0'';
ss << m_Hour << ":";
if(m_Minute < 10)
ss << ''0'';
ss << m_Minute << ":";
if(m_Second < 10)
ss << ''0'';
ss << m_Second;
return ss.str();
}

I compiled, but over and over it kept crashing. I figured maybe i was an idiot so i tried returning just ''ss << "Hello!";''. When even that crashed, i went looking back to winmain (... after trying ''ss >> "Hello!";'', and another stringstream reference).

apparently the problem laid here:
sprintf(Text, "Credits: %u\n%s", App->m_Player.GetCredits(), App->m_Total.GetNow().GetString());
I was returning a std::string, sprintf didn''t know what to do so it crashed (took me a while to find this).
The solution was simple: stringstream. I quickly replaced the sprintf with the following two lines:
std::stringstream ss;
ss << "Credits: " << App->m_Player.GetCredits() << "\n" << App->m_Total.GetNow().GetString();

Next i had a problem with my m_Font: it needed a char*. According to a stringstream reference (http://www.codeproject.com/vcpp/stl/stlintroduction.asp)
quote:
stringstream
A string stream that supports insertions of elements, and elements are inserted via the overloaded operator <<. The method str() gives a reference back to the underlying string, and the c_str() can be used to get a constant pointer to the string buffer.

Naturally, i misread that as "stringstream has a c_str() function". When that didn''t work, it was easy: it did return a string which had a c_str(), giving me this code:
App->m_Font.Print(ss.str().c_str(), 536, 7);
Of course, char* != const char* ...

Next i just created a new m_Font Print member function which takes a std::string, and voila! (yay to DirectX for taking const char*''s).

Thanks for all your help guys!

Share this post


Link to post
Share on other sites
quote:
Original post by bastardos

I''m sorry zahlman, but you''re incorrect. The prototype of the sprintf is:

int sprintf ( char * buffer, const char * format [ , argument , ...] );

The second argument is the format string.


I knew that much; I figured it would work fine because there aren''t any %''s in his string. Of course, somehow I missed the part where a C-style function isn''t going to accept a std::string where a const char * is expected :/

... oh wait, he did put in the .c_str(), so it should be fine. But then what''s the point of doing the sprintf step at all?

So now then, kVandaele:

Your printing wrapper doesn''t change the string (I hope!!!) so all you really need is


App->m_Font.Print(App->m_Total.GetNowString(tempText).c_str()
, TIMEDISPLAY_XLOC, TIMEDISPLAY_YLOC);


quote:

As you can see, DrawText takes a Long Pointer to C STRing (... i think). So, even if i modified the cFont.Print class to take a std::string variable, at some point i''d have to convert anyway.



Do it as close to the 3rd-party call as possible, i.e. in your Print class in this case. This is in conformance with the principle of "Once and only once", letting you deal with nice happy std::strings all over your own code, writing, you know, actual C++ instead of C.

quote:

I get where passing the variable to it doesn''t do anything, but i do return s, and then doesn''t this line call the c_str() of the returned std::string?
App->m_Total.GetNowString(tempText).c_str()
= s.c_str() ?


The form I suggested above will make use of the return value. The line you''re proposing now doesn''t even have a valid lvalue, as far as I can tell.

But the whole thing with having to pass in a string so that you''re not returning a reference to a local variable... it''s quite valid to have a function that expects and uses a "scratch" buffer like that, but it''s IMHO rather C-stylish. The usual Java way at least would be to have getNowString take no parameters and return a newly allocated string. Though in C++ that may not be such a good idea because of the lack of built-in garbage collection (it will be hard to track that memory).

Might I suggest as a compromise: have a "static string buffer;" added to your cTotal class, which is used as the scratch space for this function. Each time the getNowString() method (now with no args - simplifying your interfaces is a good thing!) is called, it will rewrite the contents of the buffer (which is ok, because they''re normally not visible to the rest of the world) and return a reference thereto. As a bonus (of dubious value), the implementation of "getLastTimeString()" becomes trivial.

WARNING: that won''t be thread-safe. You need to either be very careful about synchronization (probably more work than it''s worth), or set up a separate buffer for each thread.

Share this post


Link to post
Share on other sites
quote:
Original post by VolkerG
To Zahlman: The rhs of the s+= is a character pointer, no std::string appearing here.


... but it''s the lhs of an operator that defines the overloaded behaviour, and apparently there exists something like

std::string & std::string::operator+= (const char *)

, otherwise the OP would be telling us that the damn thing didn''t compile. Right? o__O

Share this post


Link to post
Share on other sites
>>... oh wait, he did put in the .c_str(), so it should be fine. But then what''s the point of doing the sprintf step at all?

Because my cFont:rint was defined as such:
BOOL cFont:rint(char *Text, long XPos, long YPos, long Width, long Height, D3DCOLOR Color, DWORD Format);
And as you know, char* Text won''t accept a const char*...

However, since DrawText DOES accept a const char*, and drawtext is what''s used in cFont:rint, i added another Print member function defined as such:
BOOL cFont:rint(std::string Text, long XPos, long YPos, long Width, long Height, D3DCOLOR Color, DWORD Format);
Oh happy day

quote:
Your printing wrapper doesn''t change the string (I hope!!!) so all you really need is


App->m_Font.Print(App->m_Total.GetNowString(tempText).c_str(), TIMEDISPLAY_XLOC, TIMEDISPLAY_YLOC);

Actually, like i said before, it would complain because c_str() returns a const char*, and it needs a char*. However, (directX''s) DrawText DOES accept a const char*, so i solved it with a new Print function, like the afore mentioned.

I''m too stupid (... well, inexperienced really) to always follow "Once And Only Once", but in this case i did. I think.

Actually multiple threads is a subject still way ahead of me.

A little late now that it''s fixed with stringstream, but in theory, if the int''s are interpreted as non-text, wouldn''t a call to itoa(int) have solved the whole thing?

Share this post


Link to post
Share on other sites
quote:
Original post by kVandaele
quote:
Your printing wrapper doesn't change the string (I hope!!!) so all you really need is


App->m_Font.Print(App->m_Total.GetNowString(tempText).c_str(), TIMEDISPLAY_XLOC, TIMEDISPLAY_YLOC);


Actually, like i said before, it would complain because c_str() returns a const char*, and it needs a char*. However, (directX's) DrawText DOES accept a const char*, so i solved it with a new Print function, like the afore mentioned.


Er... you wrote cFont:rint(), yes? As long as you don't actually *do modification to the string within Print()* (and you shouldn't), you can just change your function prototype so that it takes const char * instead, and all is well. Keep using the same type the whole way along yeah? Although actually, doing the conversion within Print, and having it take std::string, is better. Good for you for following through on that.

Once-and-only-once'ing takes some practice, a keen eye for similarity, and a disgust for code bloat. It's not as hard as you might think


[edited by - Zahlman on January 27, 2004 6:17:27 AM]

Share this post


Link to post
Share on other sites
cFont:: Print. Apparently ": P" (no space) is a smilie...
I could''ve changed the Print''s first variable from char* to const char*, but i don''t know where else i''m also using it and actually feeding it a char*, which would then probably complain because char* isn''t a const char*, so to keep things simple i just added another one.

Doesn''t "Once and only once"-ing mean you can never pass by reference? (have one and only one copy of data)

Share this post


Link to post
Share on other sites