These are chat archives for symengine/symengine

16th
Apr 2015
Ondřej Čertík
@certik
Apr 16 2015 03:02
Nice. CRTP works great for this example --- did you just figure out how to have a catch all Basic case? I think you did. Great job. Should we use CRTP for all our visitors?
I.e. why don't we move the logic from Printer directly to Visit?
Isuru Fernando
@isuruf
Apr 16 2015 03:04
This way, there are three function calls, but I think the compiler optimizes it. The second commit shows the changes to eval_double to make it work. We'll need to benchmark it and see if there is slowdown.
Ondřej Čertík
@certik
Apr 16 2015 03:05
__str2__ should be renamed to __str__
Isuru Fernando
@isuruf
Apr 16 2015 03:08
yeah
Ondřej Čertík
@certik
Apr 16 2015 03:10
The crtp call is not virtual, so it can be inlined.
Isuru Fernando
@isuruf
Apr 16 2015 03:10
To move the logic from printer to visitor, the visit function has to be a templated function but a virtual function cannot be a templated function right?
Ondřej Čertík
@certik
Apr 16 2015 03:11
@cswiercz you might be interested in the above, I know you played with CRTP as well.
@isuruf it cannot. But I am still confused by all this. Why did you change visit to print in sympy/csympy@e44a637
virtual vs non virtual function clash?
Isuru Fernando
@isuruf
Apr 16 2015 03:17
nope, that's just to make it work only with a printer.
although there is a warning about a hidden function. (we can turn that off with a flag)
I mean print looks nice for a printer. if it's visit there is no problem (only a warning which we can ignore)
Ondřej Čertík
@certik
Apr 16 2015 03:19
Well warning is bad. We can just use a different generic function name.
Isuru Fernando
@isuruf
Apr 16 2015 03:19
Yeah
Ondřej Čertík
@certik
Apr 16 2015 03:20
So visit must be virtual, because otherwise it can't dispatch the type at runtime.
Isuru Fernando
@isuruf
Apr 16 2015 03:21
Yeah. Maybe there's another way.
Ondřej Čertík
@certik
Apr 16 2015 03:21
Or can it? the accept method must be virtual to dispatch at runtime.

The accept method is defined in visitor.cpp:

#define ACCEPT(CLASS) void CLASS::accept(Visitor &v) const { v.visit(*this); }

namespace CSymPy {

ACCEPT(Symbol)
ACCEPT(Add)
...

where it calls visit on the Visitor class. That doesn't have to be virtual --- I think the only reason it is virtual is because we subclass the visitor. But if we somehow use CRTP, this doesn't need to be virtual --- thus making it a lot faster.

Isuru Fernando
@isuruf
Apr 16 2015 03:37
Issue is that we cannot have accept(Visitor &v) if Visitor is a templated class.
Ondřej Čertík
@certik
Apr 16 2015 03:38
Yeah, that's what I am trying to figure out. Also it complains about incomplete Visitor class.
Ok, at least the incomplete class can be fixed.
(By fully declaring Visitor and forward declaring Add, Symbol, ...)
Ondřej Čertík
@certik
Apr 16 2015 03:45
Yeah, it complains error: templates may not be ‘virtual’
Ok, it looks like we need to keep Visitor as is.
So maybe we can introduce CRTPVisitor (perhaps with a better name), and call some generic non virtual functions, like crtp_visit. Hopefully we can find a better name.
Ondřej Čertík
@certik
Apr 16 2015 04:01
@isuruf ok, I think I understand it now. Indeed, I didn't figure out a better way. The two virtual function calls have to be there. The third CRTP call is inlined by the compiler, I am pretty sure (we should still benchmark though). In any case, the CRTPClass (Printer in your case) can be just one for all usages. When we add a new class to csympy, only Visitor and CRTPClass need to be updated. The rest of subclasses do not need to change, as long as they have a good catch all Basic method.
I think that's a great improvement over the visitor pattern that we currently have. I still like the single dispatch approach. But we can have both and users can choose to use either one for their algorithms.
Isuru Fernando
@isuruf
Apr 16 2015 04:07
Yeah, from a user's point of view, it will be easier to write a custom CRTPClass and still get speed.
Any suggestions for a name?
Ondřej Čertík
@certik
Apr 16 2015 04:13
Maybe BaseVisitor
The disadvantage of a visitor is that you need to accumulate what you are after as a member of the visitor class, while in single dispatch, the functions can have a specific function signature for the given problem, so you can return what you need from them.
Isuru Fernando
@isuruf
Apr 16 2015 04:40
What about the function name? base_visit?