dolfin team mailing list archive

["Garth N. Wells" <gnw20@xxxxxxxxx>]

Anders Logg wrote:
> On Mon, Jun 23, 2008 at 07:31:09PM +0100, Garth N. Wells wrote:
> > Dag Lindbo wrote:
> > > > Anders Logg wrote:
> > > > > On Mon, Jun 23, 2008 at 02:51:05PM +0200, Dag Lindbo wrote:
> > > > > > Anders Logg wrote:
> > > > > > > On Thu, Jun 19, 2008 at 11:41:52PM +0200, Dag Lindbo wrote:
> > > > > > > > Hello again!
> > > > > > > >
> > > > > > > > Here comes another suggestion about the DOLFIN public interface:
> > > > > > > >
> > > > > > > > Constructing time dependent functions by subclassing from Function
> > > > > > > > and
> > > > > > > > TimeDependent is very very nice. The way u.sync(t) works by storing a
> > > > > > > > pointer to t is also very clever. However, I think it is too clever
> > > > > > > >
> > > > > > > > u.sync(&t)
> > > > > > > >
> > > > > > > > Today I misunderstood the reference nature of sync(t), and wrote code
> > > > > > > > like
> > > > > > > > this:
> > > > > > > >
> > > > > > > > while(t < T){
> > > > > > > >   "compute timestep"
> > > > > > > >   t += dt
> > > > > > > >   sync_bc(t)
> > > > > > > > }
> > > > > > > >
> > > > > > > > void sync_bc(real t){
> > > > > > > >   for "all Dirichlet BCs"
> > > > > > > >      bc.sync(t)  // Pointer to stack memory gets assigned!!!!
> > > > > > > > }
> > > > > > > >
> > > > > > > > :-( Of course, no one would be foolish enough to pass a pointer to
> > > > > > > > stack
> > > > > > > > memory knowingly in such a short lived context.
> > > > > > > >
> > > > > > > > /Dag
> > > > > > > TimeDependent does not seem to be used anywhere in the code so now
> > > > > > > it's just a fancy way of storing the current time. Objects that want
> > > > > > > to store the current time could equally well just add a member
> > > > > > >
> > > > > > >   real t;
> > > > > > >
> > > > > > > I suggest we remove TimeDependent for now and add it back later when
> > > > > > > we find any use for it.
> > > > > > >
> > > > > > > ok?
> > > > > > No! It is really good feature for non-kernel applications.
> > > > > > I think there is a clear benefit for users to have this base class to
> > > > > > standardize things. It is one of only a handful situations where I
> > > > > > really like multiple inheritance!
> > > > > >
> > > > > > /Dag
> > > > > I understand, but since the interface is not used in DOLFIN, it's
> > > > > difficult to make any sense of how the class should behave.
> > > > >
> > > > > The current implementation is bound to cause trouble (as you have
> > > > > noticed) and I don't know how to fix it since I don't know how the
> > > > > class is used.
> > > > I think that the class is worth keeping and the simple change suggested
> > > > by Dag should remove the ambiguity.
> > > >
> > > > Garth
> > > I can make the (trivial) change and add a demo under
> > > function/TimeDependent/cpp
> > I just made the change, but please contribute a demo.
> >
> > Garth
>
> I'm not sure I like it but we can discuss it more later:
>
> 1. I think it should be a reference. The '&' signifies that we
> may access the address of the object just as much as '*'.

I'm rather indifferent. I like the look of 'real& t' (which is why I 
originally did it that way), but accept that we generally pass scalars 
by value throughout the code which may cause some confusion. Do you 
suggest that we leave it as it was?

> 2. It's a bit strange to first send in an object to the constructor
> and then call sync with a possibly new object. 

It's not intended to work this way. sync is for when the time has not 
been declared when an object which depends on time is created.

Garth

To me, sync() should
> take the time t by value (not reference).



> Anyway. I'll make the new release as soon as the buildbots are happy
> (currently building).
>
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> DOLFIN-dev mailing list
> DOLFIN-dev@xxxxxxxxxx
> http://www.fenics.org/mailman/listinfo/dolfin-dev