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