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.

> > 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);