Code review comment for lp:~carlos-mazieri/ubuntu-filemanager-app/app-devel-pre4

> > Thanks for the fixes! A couple of things anymore:
> >
> > > > It'd be good to check the code on the whole that there's not more than
> > what
> > > I
> > > > noticed in this code-review
> > >
> > >
> > > DONE. added virtual for destructors. LocationsFactory does not have any
> > > virtual method nor any descendent class.
> >
> > But LocationsFactory inherits QObject which does have virtual methods so
> it's
> > a potential hard to find error even if it's not a problem in code-base
> > currently. Consider for example Qt's resource management where parent
> QObject
> > is responsible for deleting its children. If LocationsFactory (or any other
> > similar class) is set as a child of some other QObject (practically speaking
> > using the new LocationsFactory(QObject *parent) constructor where parent is
> > not null), then when the parent QObject is deleted then LocationsFactory's
> > destructor is not called - unless you make it virtual.
> >
>
> I agree that it is a good programming practice and I can do it, no problem.
> But I am not sure it happens, QObject DOES HAVE virtual destructor which in
> my opinion makes all descendant destructors to be called.

You're right. Shame on me forgetting that when base class has virtual destructor, it makes all descendant classes destructors virtual automatic even without specifically declaring them such.

So yes, it's not a problem.

>
> > > I am used to use "if (pointer)" when asking for avalid address and "if
> > > (pointer == 0)" when asking if a pointer is NULL, I could use "if (pointer
> > !=
> > > 0)" when asking for avalid address, but not "if (!pointer)" which looks
> like
> > > the variable is a bool.
> >
> > That makes sense, I misread that line. It's good.
> >
> > > >
> > > > 4)
> > > >
> > > > This perplexes me:
> > > >
> > > > void Location::setFromInfoItem(const DirItemInfo *itemInfo)
> > > > 917 +{
> > > > 918 + if (m_info)
> > > > 919 + {
> > > > 920 + delete m_info;
> > > > 921 + }
> > > > 922 + m_info = const_cast<DirItemInfo*> (itemInfo);
> > > > 923 +}
> > > >
> > >
> > > DONE. Changed to Location::setInfoItem(DireItemInfo *itemInfo) (const
> > removed)
> >
> > There's still two places with unnecessary const_casts:
> >
> > 143 + mCurLocation = const_cast<Location*> (location);
> > 235 + mCurLocation = const_cast<Location*> (location);