Re: completed my Varaint class.

From: Jumbo (nospam)
Date: 01/20/04 

Date: Tue, 20 Jan 2004 00:58:45 -0000

"Arthur J. O'Dwyer" <ajo@nospam.andrew.cmu.edu> wrote in message
news:Pine.LNX.4.58-035.0401191521210.11096@unix45.andrew.cmu.edu...
>
> On Mon, 19 Jan 2004, Jumbo wrote:
> >
> > Hi there,
> > I have completed the variant class I have been working on and I wonder
if
> > anyone could try and break it for me to see if there's anything I have
> > overlooked or screwed up on.
>
> The only real compiler errors are pretty trivial; things like
> encroaching on the implementation's namespace or forgetting to return
> a value from a function. The design of the program is what's really
> messed up -- it seems like a really ad-hoc and non-extensible method
> of getting a heterogeneous array. In real life, you wouldn't need
> these classes in the first place; and if you wanted to play with the
> heterogeneous-array idea anyway, you'd probably look through the
> [non-standard but quite popular] Boost library.
>
> > It is a wee bit long for the casual reader so please be warned:
>
> It's longer with my comments interspersed. ;-)
>
> > Here's the class etc:
> > /* User defined type simply for testing purposes */
> > class UD{
> > public:
> > UD(){std::cout<<"UD constructor"<<'\n';}
> > };
> >
> > #ifndef _STRING_
> > #include <string>
> > #endif
>
> Two errors: first, the dependence on the #definedness of an identifier
> which is /a priori/ out of your control (_STRING_ is part of the
> implementation's namespace and thus cannot be depended upon to have any
> useful value at any point in the program); and second, the way you're
> only #including <string> if some identifier has *not* been #defined.
> Occasionally, conditional compilation might see a programmer write
>
> #ifdef USE_STDSTRING
> #include <string>
> #endif
>
> but this #ifndef thing is weird, and thus probably wrong. I think you
> meant to write
>
> #include <string>
>
>
> > #ifndef _VECTOR_
> > #include <vector>
> > #endif
>
> Ditto: lose the #ifndef...#endif.

Ok I wasn't sure about this it just seemed to work ok so thanks.
I wanted to check that the headers had not been included earlier in the
program so as not to doubly include them.

So I reckoned the ifndef include the header. If I simply include the header
is not a chance that it could be included twice?

>
>
> > /* forward declaration*/
> > class DataType;
> >
> > class Base{
> > public:
> > virtual ~Base()=0{}
> > virtual void setData(int){}
> > virtual void setData(double){}
> > virtual void setData(std::string){}
> > virtual void setData(UD){}
> > };
>
> Here's another couple of problems, although these are more design-
> oriented. First, note that your class can *by design* accept only
> ints, doubles, strings, and UDs. What happens when the user wants
> to use a heterogeneous array containing floats or chars or complexes
> or FooBarBazzes? He's just out of luck, right?
> Secondly, there's no need to use inheritance for this project.
> You could lose the 'Base' class entirely, and not have any problems,
> as far as I can tell. The inheritance gains you nothing.

It's kept quite bare to reduce the size for now but it is designed in such a
way so you can add any types you like.
The idea is to have it with all the standards types initially and then the
user will need to edit the code to include a UDT but I realise this is a bit
crap so I am looking into using overloaded template constructors to create
Variants with UDT's.
>
>
> > class Variant{
> > public:
> > Variant():itsArray(0),itsLen(0){}
>
> Presumably you are going to add some 'resize' methods to this
> class once you get it working -- otherwise this constructor makes
> no sense. (If not, you might as well explicitly make 'itsLen'
> a constant value.)
> Also, you should really consider the benefits of std::vector.
> Managing a bunch of individual 'Base *' objects in an array will
> quickly become tedious.

The idea was to keep it as a fixed size but a resizable would be possible.
itsLen is initialised to 0 when the defualt constructor is called, to save
any confusion, so I know its's always going to be initialised.
>
> > Variant(const std::vector<std::string> &v1);
> > ~Variant();
> > Variant(const Variant& rhs);
> > Variant& operator=(const Variant& rhs);
> > size_t getLen()const{return itsLen;}
>
> Have you #included <cstdlib> or <stdlib.h> and I didn't notice,
> or is 'size_t' defined somewhere else in C++?
> Also, why 'getLen' and not 'length', to mimic the interface of
> C++'s standard vector class, 'std::vector'? Making your user
> remember two names for the same concept is a Bad Thing.

 I don't need to include stdlib.h to use size_t. I guess it must be included
in one of the other headers or...
Is this implementation specific?

std::vector uses size() not length ;-)
I suppose I could make is size.

>
> > template<typename T>
> > void setData(int ind, T val){
> > if(ind >=0 && ind<itsLen)
> > if(itsArray[ind])itsArray[ind]->setData(val);
> > }
> >
> > template<typename T>
> > T getData(int ind){
> > if(ind >=0 && ind<itsLen)
> > if(itsArray[ind])
> > return static_cast<DataType<T>* >(itsArray[ind])->getData();
>
> ...else, return what? You should have received a compiler diagnostic
> for this error; it should not have slipped into your final code. I
> suggest searching Google Groups for ways to portably return a 'nothing'
> value from a templated function; or else just throwing an exception.
> But if you leave it like this, both the 'if's are wasted -- you get
> undefined behavior anyway.

This is a check for nullity, although the index could be valid, if the user
had supplied an invlaid string parameter, then a null index is created.
So what I should've wrote is :
if(itsArray[ind] != 0);
This would be clearer.
>
> > }
> >
> > private:
> > std::vector<std::string> itsTypes;
> > size_t itsLen;
> > Base** itsArray;
>
> Vector for 'itsTypes', but array for 'itsArray'? That's weird...

I had a good reason to not use vector for itsArray, can't remeber what it
was right now though.
>
> > template<typename T>
> > class DataType: public Base{
> > public:
> > DataType():data(0){}
> > ~DataType(){}
> > void setData(T val){data=val;}
> > T getData(){return data;}
> > private:
> > T data;
> > };
> > };
> >
> > /* Specialized DataType constructors*/
> > template<>
> > Variant::DataType<std::string>::DataType():data("0"){}
> > /* resolves error caused if std::string is initialised to 0*/
>
> Why "0" and not ""?

Good idea, that's better.
>
> > template<>
> > Variant::DataType<UD>::DataType(){}
> > /* resolves error caused if std::string is initialised to 0*/
>
> This comment makes no sense. Bad commenting is even worse
> than no commenting.
That's not meant to be there.
>
>
> > /* Variant constructor */
> > Variant::Variant(const std::vector<std::string> &v1)
> > :itsArray(new Base*[v1.size()]),
> > itsLen(v1.size()),
> > itsTypes(v1)
> > {
> > for(size_t i=0;i<v1.size();i++){
>
> Nitpick: ITYM 'std::vector<std::string>::size_type i = 0; ...'
> [which I think can be improved to legibility by adding a typedef
> inside this function, but I'm not absolutely sure about C++ typedef
> scoping rules].

Or maybe an iterator would be better.
>
> > if(v1[i]=="int")itsArray[i] = new DataType<int>;
> > else if(v1[i]=="double")itsArray[i] = new DataType<double>;
> > else if(v1[i]=="std::string")itsArray[i] = new DataType<std::string>;
> > else if(v1[i]=="UD")itsArray[i] = new DataType<UD>;
> > else itsArray[i] = 0;
>
> You place much trust in your users. I would never expect the user
> to spell perfectly, or to guess that "std::string" was the correct
> way to refer to 'string' even after the user had explicitly declared
> that he was 'using std::string;'. This is a design problem that
> won't be solved by hacking around in this function alone; a good
> program would have a nice robust method of handling user error.
>
Well there has to be some way of telling it which datatypes to use, I came
to the conclusion that a vector of strings was the best way, if they make
and error it wouldn't crash it would simply initialise that index to null,
which reminds me I intend to make function to change types etc.So for
example if an index was null or not the desired type you could change it.
i.e: change indexes types individually. That's simple to implement here.

But I guess I should change the string initialiser to simply string as it's
easier for a user to type.
Also I intend overloading the constructor so it can be initialised with not
only a vector of strings, but I have yet to decide which would be generally
best for the user.

> > }
> > }
> >
> > /* Variant copy constructor */
> > Variant::Variant(const Variant& rhs)
> > :itsLen(rhs.getLen()),
> > itsArray(new Base*[itsLen]),
> > itsTypes(rhs.itsTypes)
> > {
> > for(size_t i=0;i<itsLen;i++){
> > if( rhs.itsTypes[i]=="int"){
> > itsArray[i] = new DataType<int>;
> > itsArray[i]->setData(static_cast<DataType<int>*
> > >(rhs.itsArray[i])->getData());
> > }
> > else if(rhs.itsTypes[i]=="double"){
> > itsArray[i] = new DataType<double>;
> > itsArray[i]->setData(static_cast<DataType<double>*
> > >(rhs.itsArray[i])->getData());
> > }
> > else if(rhs.itsTypes[i]=="std::string"){
> > itsArray[i] = new DataType<std::string>;
> > itsArray[i]->setData(static_cast<DataType<std::string>*
> > >(rhs.itsArray[i])->getData());
> > }
> > else if(rhs.itsTypes[i]=="UD"){
> > itsArray[i] = new DataType<UD>;
> > itsArray[i]->setData(static_cast<DataType<UD>*
> > >(rhs.itsArray[i])->getData());
> > }
> > else itsArray[i] = 0;
> > }
> > }
>
> Ditto this function: yuck!
It doesn't look quite so yuck when it's has not been word wrapped.
But there's no other way to initialise different types except if the type
parameters are passed as template arguments
And I don't think this would be suitable for many users, this would be
better implemented as a different class altogether.

What I mean is that I have to write DataType<int>, DataType<std::string> etc
etc as I cannot replace the type with anything other than set code.
So there needs to be a branch for each of the supported types, unless type
parameters were passed as template args.
>
> > /* Variant destructor */
> > Variant::~Variant(){
> > for(size_t i=0;i<itsLen;i++){
> > delete itsArray[i];
> > }
> > delete itsArray;
>
> Bug: you meant 'delete [] itsArray;' This is one of C++'s biggest
> pitfalls, IMHO, since only the cleverest compilers can catch even
> the most common occurrences of this bug.

Yes I should've used used delete[] although it makes no diference on my
implementation, I dunno if this is true on evey implementation though.
>
> > itsArray =0;
> > }
> >
> > /* Variant operator= */
> > Variant& Variant::operator=(const Variant& rhs){
> > itsTypes =rhs.itsTypes;
> > if(itsLen != rhs.getLen()){
> > this->~Variant();
> > itsLen = rhs.getLen();
> > itsArray = new Base*[itsLen];
> > for(size_t i=0;i<itsLen;i++){
> > if( rhs.itsTypes[i]=="int")itsArray[i] = new DataType<int>;
> > else if(rhs.itsTypes[i]=="double")itsArray[i] = new DataType<double>;
> > else if(rhs.itsTypes[i]=="std::string")itsArray[i] = new
> > DataType<std::string>;
> > else if(rhs.itsTypes[i]=="UD")itsArray[i] = new DataType<UD>;
> > else itsArray[i] = 0;
> > }
> > }
> > for(size_t i=0;i<itsLen;i++){
>
> These two loops should be executed in parallel inside a single loop,
> not split up like this. Write instead,
>
> for (size_t i=0; i < itsLen; ++i) {
> if (rhs.itsTypes[i] == "int") {
> itsArray[i] = new DataType<int>;
> itsArray[i]->setData(static_cast<DataType<int>*>
> (rhs.itsArray[i]->getData());
> }
> else if (rhs.itsTypes[i] == "double") {
> [...]
There is a reason why they are split up. It was to save the need for
re-allocation if the objects were both the same size but you have pointed
out an error here.
If they are the same size but different types it crashes so I need to check
that the types match too.
I think now is a time to implement my changeType methods so I can use it to
change the DataTypes here.
>
> > if( rhs.itsTypes[i]=="int")
> > itsArray[i]->setData(static_cast<DataType<int>*
> > >(rhs.itsArray[i])->getData());
> > else if(rhs.itsTypes[i]=="double")
> > itsArray[i]->setData(static_cast<DataType<double>*
> > >(rhs.itsArray[i])->getData());
> > else if(rhs.itsTypes[i]=="std::string")
> > itsArray[i]->setData(static_cast<DataType<std::string>*
> > >(rhs.itsArray[i])->getData());
> > else if(rhs.itsTypes[i]=="std::string")
> > itsArray[i]->setData(static_cast<DataType<UD>*
> > >(rhs.itsArray[i])->getData());
> > }
> > return *this;
> > }
> >
> > Right, that's all the hard stuff out of the way , I saved it in my
include
> > folder an named it vector.
>
> Huh?
>
> > Now here's a little test program to demonstrate its usage:
> >
> > #include <iostream>
> > #include <variant>
> >
> > int main(){
> > std::vector<std::string> strArr1(5);
> > strArr1[0]= "int";
> > strArr1[1]= "std::string";
> > strArr1[2]= "double";
> > strArr1[3] = "UD";
>
> ...and strArr1[4] equals...? You invoke undefined behavior later,
> by trying to evaluate the contents of strArr1[4] without having put
> anything there.

Ah this would be a user error. What would happen here is that a Variant of
length 5 would be created but the last two indexes would be null.
So I have guarded against this user error.
Once I have implemented a setType methodthe user can initialise the Variant
with some null indexes and then later can initialise them nulls.
So yes attempting to access a null DataType would produce undefined
behavior, but this is how it is supposed to be.
At least it has bounds checking.
>
> > Variant v1(strArr1);
> > v1.setData(0,5);
> > v1.setData(1,"Hello");
> > v1.setData(2, 6.5);
> > v1.setData(3, 5); /* does nothing to UD */
> >
> > std::vector<Variant> vv(2);
>
> It's also kind of weird that here you define a vector of two
> Variants, but you only use one of them. Sure, it's a test
> program, but you weren't seriously thinking that your class might
> break if put into a vector, were you??

It was just an example, what I was testing here was the operator= but I
didn't test it thoroughly enough .:-s
As your comments have enlightened me, if I try to assign a Variant of same
size but different tpyes it breaks.
But I'll easy sort that :-)
>
> > vv[0] = v1;
> >
> > std::cout<< vv[0].getData<int>(0) <<'\n';
> > std::cout<< vv[0].getData<std::string>(1) <<'\n';
> > std::cout<< vv[0].getData<double>(2) <<'\n';
> > vv[0].getData<UD>(3);
>
> Ouch! Here's the biggest problem, which I hadn't even been
> considering until it was right in my face: You're setting up this
> big, complicated data structure with inheritance and string
> comparisons and template functions and all that, and after all
> that work, you're still making your client remember what types
> he's stored in the array?! That's just mean!
> I could get exactly the same behavior you've demonstrated here
> in five minutes with a managed vector of (void*). And so could
> anyone who wanted to -- including yourself! So what purpose does
> this code serve, other than an example of uber-bad practice?

No you couldn't, you couldn't get the same type checking,functionatily and
encapsulation with a vector of void*.
Anyhow the need to state which kind of datatype to return is necessarry to
know, in a real program you are going to be doing something other than
printing it the screen and you'd need to know this anyway. Besides it's a
safeguard against misuse of type as when the uses tries to access a dataype
of the wrong type they'll get back something null. But saying that an error
could escape a users attention if they expected to get null back so
therefore I should really throw an exception if wrong type access is
specified.
The beauty of this is that you simply pass a template parameter, whereas if
you had a vector of void* you'd be farting around with casts and allsorts,
so you'd still need to know the type of data anyway but there'd be no
safeguards with void*'s.

>
> > /*try to break it etc etc*/
> > return 0;
> > }
> >
> > Obviously the class is basic and would need to be adapted for project
> > specific use.
> > If anyone can spot any errrors please let me know.
>
> See the previous sentence: bad design is itself an error.
lol
>
> > Also be obliged if anyone can see a way to overload the subscripts so as
to
> > return different types.
>
> I'm sure Boost has some clever way to minimize the client's workload,
> but I'm not at all sure that C++ lets you do this; it is, after all, a
> statically typed language.
Yes I'm sure boost is very good but I am doing this as a learning project
and I don't feel ready to learn boost as there's alot of the STL I have yet
to learn first.

Also I was on about using templated parameters as the tpye args earlier,
there is a good example in the book C++ in a nutshell of using templates to
create type lists so I might move over to that design sooner that I though.
It's page 206 is anyones intersted.
>
TFYH.

Relevant Pages

  • Re: Code to delete/unlink Linked tables
    ... Public intLinkODBCTables As Variant, intLinkDB2Tables As Variant ... Public strLinkBackendDB As String, strLinkDSNname As String, strLinkLibName ... ' MsgBox "This database is in MDE format...I will delete/recreate ODBC ... Public Sub fncLinkDB2Table() ...
    (microsoft.public.access.modulesdaovba)
  • Re: OPENFILENAME A P I. User cancels ?
    ... The variable FName will be a Boolean False if no file was selected or a String containing the fully qualified file name if the user selectes a file. ... Const ahtOFN_OVERWRITEPROMPT = &H2 ... Function GetOpenFile(Optional varDirectory As Variant, ... Dim strFilter As String ...
    (microsoft.public.excel.programming)
  • RE: multi-select file dialog
    ... strCustomFilter As String ... Global Const ahtOFN_OVERWRITEPROMPT = &H2 ... Dim strFilter As String ... Function GetOpenFile(Optional varDirectory As Variant, ...
    (microsoft.public.access.modulesdaovba)
  • Re: Replace space in text file
    ... In four of the above cases you have specified which type of variable you want VB to create (Integer, Long, String or whatever). ... in the case of vChars you have not specified the required variable type and VB will therefore create it as a Variant. ... For example, if vChars was an Integer then VB would read two bytes from the file, and if it was a Long it would read four bytes, and if it was a standard variable length String which currently contained nine characters then it would read nine characters from the file and if it was a String that currently contained just one character then it would read one character from the file. ...
    (microsoft.public.vb.general.discussion)
  • RE: multi-select file dialog
    ... strCustomFilter As String ... Global Const ahtOFN_OVERWRITEPROMPT = &H2 ... Dim strFilter As String ... Function GetOpenFile(Optional varDirectory As Variant, ...
    (microsoft.public.access.modulesdaovba)