wrote:
> Hi,
>
> I'm looking at a legacy string class thats been in use here for a while
> and I'd like to check out any options available to optimise it. I see a
> couple of constructors that look dubious. Consider the following ctor.
> It constructs a TtkString object with a string value of the integer
> contained withing. e.g
>
> TtkString one(456);
> cout << one << endl;
>
> prints;
>
> 456
Constructors that accept a single, int parameter are usually best
declared "explicit". Otherwise, the implicit conversions - especially
from 0 - can be unexpected.
> // "string" is declared as a const char* within the class
> TtkString::TtkString(int i)
> {
>
> std::stringstream s;
> s << i << std::ends;
> std::string myString = s.str();
> const char* localString = myString.c_str();
> int size(strlen(localString));
> string = (char *) malloc((size + 1)*sizeof(char)) ;
> memset(string,0,size+1);
> strncpy(string, localString,size);
>
> }
First, "string" is poor choice for a member name - especially of a
string class that uses std::string's to some extent. So I would
redeclare the member variable to be a std::string and give it a
different name.
Now concerning the current implementation: this constructor starts out
OK. Granted, stringstream is a bit heavyweight. On the other hand, C++
is not blessed with an over abundance of convenient routines for
converting between numbers and strings. And none other than Bjarne
himself recommends using stringstream for this purpose. Now, I would be
much more concerned about the sudden, nightmarish turn for the worse
that the constructor takes, managing to call malloc(), memset(),
strncpy() - a veritable rogue's gallery of C's unsized, untyped
operations that have no business threatening our C++ code.
> I was considering using something like this for the body of the same
> function....
>
> string = (char*) calloc (1, 33); // 32 bit system assumed.
> memset(string,0,33);
> itoa(i, string, 10);
First, itoa() is a non-standard routine. Furthermore, since calloc()
returns zero-initialized memory there is no point in zeroing out the
memory a second time. And what is the rationale for the magic number
33? Generally choosing a power of two would make a lot more sense given
that computers are binary machines. Besidss, a 33 digit number is a bit
excessive. I am not sure whether even a 128-bit long double has that
many digits of precision.
I would just stick with the std::stringstream and copy its std::string
to a std::string member variable (replacing the const char pointer) as
mentioned above. If you do decide to replace stringstream, then I would
use a standard routine with a sized, character buffer, such as
snprintf(), and then copy the buffer into a std::string.
> however this is not working....Ive obviously messed something up. Can
> anybody shed some light? My approach allocates 33 bytes regardless of
> what the "i" argument is... e.g if its 1 then I don't need all 33
> bytes, do I really?
The best idea is to delegate memory handling chores to a class object
like std::string. There is no other change worth making until all of
the calls to malloc, memset, memcpy and their ilk have been eliminated
by one means or another.
Greg
Similar Threads
