Re: Add and use a Transform data type (issue 344970043 by address@hidden)

[dak]

https://codereview.appspot.com/344970043/diff/40001/lily/include/transform.hh
File lily/include/transform.hh (right):

https://codereview.appspot.com/344970043/diff/40001/lily/include/transform.hh#newcode52
lily/include/transform.hh:52: Transform (Offset p0)
On 2018/06/20 22:30:07, Dan Eble wrote:
> Do you want to allow implicit conversion from an Offset to a
Transform?  If not,
> this should be "explicit".

Done.

https://codereview.appspot.com/344970043/diff/40001/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/344970043/diff/40001/lily/stencil-integral.cc#newcode740
lily/stencil-integral.cc:740: if (Transform *tp = unsmob<Transform>
(trans))
On 2018/06/20 22:30:07, Dan Eble wrote:
> You've cleaned up a lot, but this recurring optionality could probably
be
> improved.  Maybe a function would help:

>      Transform make_transform(const Transform *t)
>        {
>          return t ? Transform (*t) : Transform ();
>        }

> You could also do it as a constructor, if you prefer its syntax and
don't mind
> implementing yet another one:

>      explicit Transform(const Transform *t) ...

> I merely suggest.

A constructor/function from Transform * seems like a side track when the
only use case is conversion from SCM.  A constructor avoids an extra
copy (but I think C++19(?) or so already states that returned classes
are to be considered initializers rather than temporaries).  But it
would seem weird to have a constructor/convertor that silently takes a
non-Transform SCM value to an identity transform.

What would your favorite be here?  It's not like this pattern occurs
often enough (yet) to really require something here or right now.

https://codereview.appspot.com/344970043/