Re: [Libreoffice] [PATCH] Removed dependencies on tools/solar.h in drawpage.hxx/.cxx

On Wed, Jan 5, 2011 at 12:56 PM, Soeren Moeller
<soerenmoeller2001@gmail.com> wrote:
> Hi and thank you for your comments
>
> 2011/1/5 Kohei Yoshida <kyoshida@novell.com>:
> > Hi Sören,
> > > +#include <sal/types.h>
> > >  #include "address.hxx"
> >
> > This should not be necessary (though it won't hurt) as the address.hxx
> > already includes sal types.  Does it not build without this change for
> > you?
> It does build without the change for me, I did the change to ensure
> that we don't break the file accidentially by changing includes of
> address.hxx, although I now can see that this maybe isn't necessary
> for such basic includes (types are quite standard and almost everyone
> includes address.hxx). I think I felt it more important yesterday
> after reading a lot of files using things from tools/ but only
> including them through other includes, which would break the code, if
> I would remove the tools/ parts from the other includes first.

I think that including what you need where you need it is a good practice.
replying on unrelated included to bring 'magically' the other includes
you need is a maintenance pain.
(with the exception of a set of known and documented 'meta-include',
which we do not really have)

I do think that solar.h related stuff should be such a mandatory
meta-include that should be the first include of every sources of
ours.
That would make easy to deal with type migration and
compatibility-hack, by giving an entry point where to put these.

Norbert

> > >  // Let's put here misc structures that get passed to ScDocument's methods.
> > > diff --git a/sc/inc/drawpage.hxx b/sc/inc/drawpage.hxx
> > > index faa43fa..5e207b7 100644
> > > --- a/sc/inc/drawpage.hxx
> > > +++ b/sc/inc/drawpage.hxx
> > > @@ -30,7 +30,7 @@
> > >  #define SC_DRAWPAGE_HXX
> > >
> > >  #include <svx/fmpage.hxx>
> > > -
> > > +#include <sal/types.h>
> > >
> > >  class ScDrawLayer;
> > >
> > > @@ -39,7 +39,7 @@ class ScDrawLayer;
> > >  class ScDrawPage: public FmFormPage
> > >  {
> > >  public:
> > > -    ScDrawPage(ScDrawLayer& rNewModel, StarBASIC* pBasic, BOOL bMasterPage=FALSE);
> > > +    ScDrawPage(ScDrawLayer& rNewModel, StarBASIC* pBasic, sal_Bool bMasterPage=sal_False);
> > >      ~ScDrawPage();
> > >
> > >      virtual ::com::sun::star::uno::Reference< ::com::sun::star::uno::XInterface > 
> > > createUnoPage();
> > > diff --git a/sc/source/core/data/drawpage.cxx b/sc/source/core/data/drawpage.cxx
> > > index d489d5d..5a91540 100644
> > > --- a/sc/source/core/data/drawpage.cxx
> > > +++ b/sc/source/core/data/drawpage.cxx
> > > @@ -44,7 +44,7 @@
> > >
> > >  // -----------------------------------------------------------------------
> > >
> > > -ScDrawPage::ScDrawPage(ScDrawLayer& rNewModel, StarBASIC* pBasic, BOOL bMasterPage) :
> > > +ScDrawPage::ScDrawPage(ScDrawLayer& rNewModel, StarBASIC* pBasic, sal_Bool bMasterPage) :
> > >      FmFormPage(rNewModel, pBasic, bMasterPage)
> > >  {
> > >      SetSize( Size( LONG_MAX, LONG_MAX ) );
> >
> > For these two, let's use the standard bool type.  In fact, its parent
> > class FmFormPage does expect a parameter of the bool type, not sal_Bool
> > nor BOOL (though the default value is for some reason set to sal_False
> > instead of false, which should really be fixed...), which provides
> > another reason to use bool, not sal_Bool.
> I think I'm still too afraid of breaking UNO-things, if there just is
> something UNO-related close to the code, when using native types, I
> will try to be more confident in using bool, and believing in it, if
> my build works out fine.
>
> Regards
> Sören
>
>
> > Thanks!
> >
> > Kohei
> >
> > --
> > Kohei Yoshida, LibreOffice hacker, Calc
> > <kyoshida@novell.com>
> _______________________________________________
> LibreOffice mailing list
> LibreOffice@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libreoffice