Now the only failure seems to be for optional MPFR and Arb. In fa17763 I implemented visitor like approach, for a new method diff2. Once I implement all derivatives, I'll get rid of diff and rename diff2 to diff. I posted my current status here, so that you can see the design. It's using static methods, but the diff2 method can just as well accept an instance of DiffImplementation and call regular non-static methods on it, then the instance can save intermediate data inside it, just like the visitors work. Also since I use the type codes macros trick, this takes care of the optional MPFR, Arb, etc. issues.

So now the only difference between a visitor pattern and this is that the visitor pattern allows the user to implement new functionality without touching the core at all, for the price of one extra virtual function call. This PR gets rid of the extra virtual function call, but it requires to add a new method to basic-methods.inc for each new algorithm (and thus full recompilation), but that's the only modification to our header files. The rest is then implemented in a .cpp file and thus is fully local.

And so I think we can use this approach to implement critical core functionality where we require maximum speed. We'll still allow the visitor approach, so that users can try out new things without modifying the core. We should also see if there is a way to unify the visitor class with the DiffImplementation class, so that one can (perhaps) just subclass a different class to switch back and forth between a visitor pattern and this new approach.

@isuruf and others, let me know what you think.

I finished the implementation and removed all old diff methods and instead use the new ones. Tests pass on Travis, the Appveyor failures are unrelated (we should however fix them to make sure this PR builds with MSVC).

As to benchmarks, the only diff benchmark that we have so far is symbench S3 and S3a, neither of which seems to be affected.

This is ready for a review.

@certik, there's another way to implement this with little change.

For each class we add methods like

accept(StrPrinterFinal &v) {
     v.bvisit(*this);
}

where StrPrinterFinal is defined as follows.

class StrPrinterFinal : public BaseVisitor<StrPrinterFinal, StrPrinter> {
     using StrPrinter::bvisit;
}

and then it should work without other changes.

This way we keep the functionality of Visitors. Visitors with no specific accept methods will have a cost of two virtual function calls. Others will have a cost of only one virtual function call.

@isuruf indeed, we can now do that too. Do you have any objections against merging this PR?

As to the Appveyor failure, I reported it here: http://help.appveyor.com/discussions/problems/3934-cannot-open-include-file-stdioh-no-such-file-or-directory (update: it's been fixed in Appveyor and I restarted the build)

Looks good to me. +1 to merge after tests pass

I pushed in two more patches that implement your idea for eval_double. Indeed it works. I benchmarked with this patch:

diff --git a/benchmarks/eval_double1.cpp b/benchmarks/eval_double1.cpp
index 4bcda19..92e4451 100644
--- a/benchmarks/eval_double1.cpp
+++ b/benchmarks/eval_double1.cpp
@@ -42,7 +42,7 @@ int main(int argc, char* argv[])

     auto t1 = std::chrono::high_resolution_clock::now();
     for (int i = 0; i < 500; i++)
-        r = eval_double(*e);
+        r = eval_double_single_dispatch2(*e);
     auto t2 = std::chrono::high_resolution_clock::now();
     std::cout
         << std::chrono::duration_cast<std::chrono::milliseconds>(t2-t1).count()

without it I got:

certik@redhawk:~/repos/symengine/benchmarks(methods)$ ./eval_double1 
4620ms
r (double) = 9.85647
r (exact)  = 9.85647
error      = 5.32907e-15
certik@redhawk:~/repos/symengine/benchmarks(methods)$ ./eval_double1 
4758ms
r (double) = 9.85647
r (exact)  = 9.85647
error      = 5.32907e-15
certik@redhawk:~/repos/symengine/benchmarks(methods)$ ./eval_double1 
4773ms
r (double) = 9.85647
r (exact)  = 9.85647
error      = 5.32907e-15
certik@redhawk:~/repos/symengine/benchmarks(methods)$ ./eval_double1 
4724ms
r (double) = 9.85647
r (exact)  = 9.85647
error      = 5.32907e-15

with it I got:

certik@redhawk:~/repos/symengine(methods)$ ./benchmarks/eval_double1 
4698ms
r (double) = 9.85647
r (exact)  = 9.85647
error      = 5.32907e-15
certik@redhawk:~/repos/symengine(methods)$ ./benchmarks/eval_double1 
4680ms
r (double) = 9.85647
r (exact)  = 9.85647
error      = 5.32907e-15
certik@redhawk:~/repos/symengine(methods)$ ./benchmarks/eval_double1 
4674ms
r (double) = 9.85647
r (exact)  = 9.85647
error      = 5.32907e-15
certik@redhawk:~/repos/symengine(methods)$ ./benchmarks/eval_double1 
4594ms
r (double) = 9.85647
r (exact)  = 9.85647
error      = 5.32907e-15

So it seems it is a bit faster.

I first tried to overload accept, and it compiled, but it was still using the two virtual calls, because the apply method was at compile time dispatching to the general accept. This is fixed by implementing apply again in the EvalRealDoubleVisitorFinal. However, because it is so easy to get this wrong, I decided to rename the specialized accept to accept_EvalRealDoubleVisitorFinal, then there is no confusion about which method is being called.

In 519e467 I ported eval_complex_double() to show how easy it is.

I am done with this PR. The rest we can do in new PRs. @isuruf are you ok with the new patches as well?

Hm, but the apply calls implemented e.g. in EvalDoubleVisitor::bvisit(const Pow &x) still call EvalDoubleVisitor::apply, that calls Basic::accept(Visitor &v) which is slow...

@certik, yes, let's first get 6963b8c in and then see how to improve

Indeed. I reseted to 6963b8c. I pushed the other patches into https://github.com/certik/symengine/tree/methods5

I did push in 6201e78, since this should be part of this PR. This provides a solid foundation for our future PRs.

Looks good to me too.

Btw, here is how to port StrPrinter on top of this PR: certik@1fa371e and all tests pass, except the MyStrPrinter ones, because the bvisit(const Sin &x) method is never called -- previously I think the bvisit binding is determined at compile time of MyStrPrinter, while now it is determined at compile time of StrPrinter. So the extensibility is limited, but the StrPrinter itself works great.

AppVeyor got stuck, I created #740 for that.

See 98465c6, which builds on top of your str branch.

+1