dolfin team mailing list archive

[Anders Logg <logg@xxxxxxxxx>]

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 '*'.

2. It's a bit strange to first send in an object to the constructor
and then call sync with a possibly new object. 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).

-- 
Anders

Attachment: signature.asc
Description: Digital signature