Re: why not use naked delete ?
On Wednesday, December 31, 2014 1:10:18 AM UTC+2, Lynn McGuire wrote:
On 12/30/2014 4:13 PM, =E5=98=B1 Tiib wrote:
On Tuesday, December 30, 2014 11:32:33 PM UTC+2, Lynn McGuire wrote:
On 12/29/2014 10:29 PM, Richard wrote:
[Please do not mail me a copy of your followup]
Lynn McGuire <lmc@winsim.com> spake the secret code
<m7s3fr$df9$1@dont-email.me> thusly:
We use new and delete a lot in our base classes since we are memory
constrained in the Win32 environment.
By "memory constrained" am I to infer you are talking about stack
space?
We run out of Win32 space if we are not careful. Probably heap space
since some of our datasets go over one GB. Things are better
not though since we started compressing strings in memory.
Why naked new and not std::unique_ptr<> or some sort of container?
This code was written over a decade ago and works well. Never rewrite
code that is working well just to use new coding features.
There is difference between writing:
"We used new and delete a lot in our base classes that we wrote over
decade ago."
Or:
"We use new and delete a lot in our base classes since we are memory
constrained in the Win32 environment."
First is common, lot of people did it over decade ago. Second is
intriguing because writing naked 'new' and 'delete' in modern C++
adds complexities (and so defects) but does not save resources.
So how would you implement this class without using new? Here is the cla=
ss and constructor using a reference value:
I do not know the purpose of it. So I can't tell for sure what I would do.
Feels it has a lot of members so possibly I would consider splitting
it up or reducing members somehow. I will comment below:
class DesValue : public ObjPtr
{
public:
int datatype; // Either #Int, #Real, or #String.
int vectorFlag; // Flag indicating value contains an Array.
int optionListName; // name of the optin list item
int * intValue; // Either nil, an Int, a Real, a String, or an Arr=
ay thereof.
double * doubleValue;
string * stringValue;
vector <int> * intArrayValue;
vector <double> * doubleArrayValue;
vector <string> * stringArrayValue;
Above part of it seems to be a variant. If it is so then instead I would
use some existing implementation of variant. For example boost::variant:
typedef boost::variant< nullptr_t,
int,
double,
std::string,
std::vector<int>,
std::vector<double>,
std::vector<std::string>
> ValueVariant;
ValueVariant variantValue;
Its copy-construction is default.
The 'datatype' can be changed to function '{return variantValue.which();}'.
The 'vectorFlag' can be changed to function '{return datatype() > 3;}''.
unsigned char * compressedData;
unsigned long compressedDataLength;
Why not vector instead of above? Like:
std::vector<unsigned char> compressedData;
Its copy-construction is default.
vector <unsigned long> uncompressedStringLengths;
std::string * uncompressedString;
Why above is pointer? I would replace it with just 'std::string':
std::string uncompressedString;
That would also copy by default. As are rest of the members.
int isTouched; // Flag indicating if value, stringValue, or units =
have been modified since this DesValue was created. Set to
true by setValue, setString, setUnits, and convertUnits.
int isSetFlag; // Flag indicating whether the contents of the DesV=
alue is defined or undefined. If isSet is false, getValue
returns nil despite the contents of value, while getString and getUnits r=
eturn the empty string despite the contents of stringValue
and units.
int unitsValue; // current string value index in $UnitsList (singl=
e or top)
int unitsValue2; // current string value index in $UnitsList (bott=
om)
string errorMessage; // message about last conversion of string to=
value
string unitsArgs; // a coded string of disallowed units
public:
// constructor
DesValue ();
DesValue (const DesValue & rhs);
Above copy constructor could be just defaulted if to do like I suggested
above since compiler-generated is as good:
DesValue (const DesValue & rhs) = default;
DesValue & operator = (const DesValue & rhs);
// destructor
virtual ~DesValue ();
Likely copy assignment and destructor and move constructor and move
assignment as well.
virtual DesValue * clone () { return new DesValue ( * this); }
Yes, that has to remain if you need virtual clone with covariance
because there are no covariance between 'std::unique_ptr<base>' and
'std::unique_ptr<derived>'.
...
}
Looks quite big snip:
DesValue::DesValue (const DesValue & rhs)
{
datatype = rhs.datatype;
vectorFlag = rhs.vectorFlag;
optionListName = rhs.optionListName;
if (rhs.intValue)
{
intValue = new int;
* intValue = * rhs.intValue;
}
else
intValue = NULL;
if (rhs.doubleValue)
{
doubleValue = new double;
* doubleValue = * rhs.doubleValue;
}
else
doubleValue = NULL;
if (rhs.stringValue)
{
try
{
stringValue = new string;
* stringValue = * rhs.stringValue;
}
catch (std::bad_alloc &ba)
{
char msg [1024];
sprintf_s (msg, sizeof (msg),
"A memory error has occurred that could not be handled.\n"
"Please try the operation again.\n\n"
"Message: %s\n"
"Size: %d bytes", ba.what (), rhs.stringValue -> size ());
alert (msg);
}
}
else
stringValue = NULL;
if (rhs.intArrayValue)
{
intArrayValue = new vector <int>;
* intArrayValue = * rhs.intArrayValue;
}
else
intArrayValue = NULL;
if (rhs.doubleArrayValue)
{
doubleArrayValue = new vector <double>;
* doubleArrayValue = * rhs.doubleArrayValue;
}
else
doubleArrayValue = NULL;
if (rhs.stringArrayValue)
{
stringArrayValue = new vector <string>;
* stringArrayValue = * rhs.stringArrayValue;
}
else
stringArrayValue = NULL;
if (rhs.compressedData && rhs.compressedDataLength)
{
unsigned long num = rhs.uncompressedStringLengths.size ();
if (vectorFlag && num)
{
// if a vector of strings, copy the uncompressed string lengths
uncompressedStringLengths.resize (num);
for (unsigned long i = 0; i < num; i++)
uncompressedStringLengths [i] = rhs.uncompressedStringLengths [i];
}
// copy the size of the compressed data
compressedDataLength = rhs.compressedDataLength;
// allocate and copy the compressed data
compressedData = (unsigned char *) malloc (rhs.compressedDataLength);
if ( ! compressedData)
alert ("DesValue::DesValue (const DesValue &) - unable to malloc " +
asString (rhs.compressedDataLength) + " bytes");
memcpy_s (compressedData, rhs.compressedDataLength,
rhs.compressedData, rhs.compressedDataLength);
}
else
{
compressedData = NULL;
compressedDataLength = 0;
uncompressedStringLengths.resize (0);
}
if (rhs.uncompressedString)
{
uncompressedString = new std::string;
* uncompressedString = * rhs.uncompressedString;
}
else
uncompressedString = NULL;
isTouched = rhs.isTouched;
isSetFlag = rhs.isSetFlag;
unitsValue = rhs.unitsValue;
unitsValue2 = rhs.unitsValue2;
errorMessage = rhs.errorMessage;
unitsArgs = rhs.unitsArgs;
}