dolfin team mailing list archive

[Johan Hake <hake@xxxxxxxxx>]

On Monday 07 September 2009 10:01:26 Garth N. Wells wrote:
> Ola Skavhaug wrote:
> > On Mon, Sep 7, 2009 at 9:21 AM, Ola Skavhaug<skavhaug@xxxxxxxxx> wrote:
> >> We've run into problems since only GenericMatrix provides the default
> >> argument for norm type. Swig does not like this and omits generating a
> >> constructor for the derived types. According to
> >> https://www.securecoding.cert.org/confluence/display/cplusplus/OBJ04-CPP
> >>.+Prefer+not+to+give+virtual+functions+default+argument+initializers
> >> virtual function should not provide default arguments at it tends to
> >> "defeat polymorphism":
> >>
> >>
> >> class Thing {
> >>  public:
> >>    virtual ~Thing();
> >>    virtual void doItNTimes( int numTimes = 10 );
> >>    virtual void doItThisHigh( double howHigh = 1.0 );
> >>    // ...
> >> };
> >> class MyThing : public Thing {
> >>  public:
> >>    void doItNTimes( int numTimes );
> >>    void doItThisHigh( double howHigh = 2.2 );
> >>    // ...
> >> };
> >>
> >> MyThing *mt = new MyThing;
> >> Thing *t = mt;
> >> t->doItNTimes(); // default of 10
> >> mt->doItNTimes();  // compile time error!
> >> t->doItThisHigh(); // default of 1.0!
> >> mt->doItThisHigh(); // default of 2.2
> >>
> >>
> >> Is this the intended behaviour anyhow?
> >>
> >> This is the proposed solution:
> >
> > Ouch, this was another, bad solution actually :)
> >
> > The proposed solution (from the link above) is roughly like this:
> >
> > class Base
> > {
> >    public:
> >       void foo(double bar = 1.0) { foo_impl(bar); }
> >    protected:
> >       virtual void foo_impl(double bar);
> > };
> >
> > class Sub: public Base
> > {
> >    protected:
> >       virtual void foo_impl(double bar);
> > };
> 
> This is pretty ugly and I know that I'll forget about it within a day or
> two. I suggest abandoning default arguments for norms if that fixes the
> problem. We have a default argument for str(..), but I think that
> PyDOLFIN has its own mechanism for this.

Yes, this is correct. This was probably added to get around this issue. We can 
add workarounds for this case too, but it becomes quite cumbersome to do it 
for each method. Doing it for a fundamental base class as Variable maybe makes 
more sense?

However note that following the report from Ola this will not work:

  PETScMatrix *pm = new PETScMatrix();
  ...
  pm.src();  // Compile error

where this would:

  PETScMatrix *pm = new PETScMatrix();
  ...
  Variable * v = pm;
  pm.str();

This has probably not turned up as a problem as we are using str() indirectly 
through info().

I can remove the default argument from

  PETScMatrix::norm

> Note that we have a related Blueprint,
> 
>      https://blueprints.launchpad.net/dolfin/+spec/default-parameters-cpp

I tried to link to a discussion for this blueprint but couldn't do that. We 
need to first register a bug on it to be able to connect it to a blueprint.

Johan

> Garth
> 
> > Sorry about that...
> >
> > Ola
> >
> >> static double const minimumHeight = 1.0;
> >> // ...
> >> namespace XYZ {
> >> class Thing {
> >>  public:
> >>    virtual ~Thing();
> >>    virtual void doItThisHigh( double howHigh = minimumHeight );
> >>    // ...
> >> };
> >> }
> >> // ...
> >> namespace XYZ {
> >> static double const minimumHeight = 2.2;
> >> class MyThing : public Thing {
> >>  public:
> >>    void doItThisHigh( double howHigh = minimumHeight );
> >>    // ...
> >> };
> >> }
> >>
> >>
> >> Resolving this issue also means getting the Python interface back on
> >> track.
> >>
> >> Ola
> >>
> >>
> >> --
> >> Ola Skavhaug
> 
> _______________________________________________
> DOLFIN-dev mailing list
> DOLFIN-dev@xxxxxxxxxx
> http://www.fenics.org/mailman/listinfo/dolfin-dev
>