Replaced uses of Simplify() with SimplifyVisitor{} #190
Conversation
404Naz
changed the title
404Naz
force-pushed
the
feature-visitor-simplify
branch
from
32623da to
19a0a00
Compare
…ge the visitor pattern and the ability to pass states with one object rather than passing a series of individual states through the simplify chain. An example of this would be degrees vs radians for trig functions. With the new visitor pattern, we can pass that option to all nodes that need it without passing additional parameters to each class's Simplify() function.
404Naz
force-pushed
the
feature-visitor-simplify
branch
from
c6a0751 to
c095399
Compare
| tinyxml2::XMLElement* result = doc.NewElement("mn"); | ||
| result->SetText(std::format("{:.5}", real.GetValue()).c_str()); | ||
| return result; | ||
| return gsl::not_null(gsl::make_not_null(result)); |
There was a problem hiding this comment.
Is it possible to make this a single gsl::make_not_null call?
| Exponent exp { expExpCase.GetMostSigOp().GetMostSigOp(), e }; | ||
| return gsl::make_not_null(exp.Copy()); | ||
| } else { | ||
| Exponent exp { expExpCase.GetMostSigOp().GetMostSigOp(), *(std::move(s.value())) }; |
There was a problem hiding this comment.
you should just be able to use *(s.value()) if you using the const& (copy) constructor. Otherwise, use std::move(s).value if you're using the && (move) constructor
There was a problem hiding this comment.
Remove the = 0 from the Simplify method so that it is no longer pure virtual (i.e abstract). The default implementation should call the new SimplifyVisitor. This is so that new Expression types don't have to implement the now deprecated Simplify.
| if (!s) { | ||
| return e.Generalize(); | ||
| } else { | ||
| return std::move(s.value()); |
There was a problem hiding this comment.
use std::move(s).value()
| if (!simp) { | ||
| return result.Generalize(); | ||
| } | ||
| return std::move(simp).value(); |
There was a problem hiding this comment.
squirrels in my pants
| } | ||
| return std::make_unique<Exponent<Expression>>(exprCase->GetMostSigOp(), | ||
| *(Add<Expression> { exprCase->GetLeastSigOp().GetLeastSigOp(), Real { 1.0 } }.Simplify())); | ||
| *(std::move(s.value()))); |
There was a problem hiding this comment.
do std::move(s).value()
| #include "Oasis/Undefined.hpp" | ||
| #include "Oasis/Variable.hpp" | ||
|
|
||
| #define EPSILON 1E-6 |
There was a problem hiding this comment.
use https://en.cppreference.com/w/cpp/types/numeric_limits/epsilon instead because it can differ per machine and between float and double
| if (!s) { | ||
| return s; | ||
| } | ||
| return gsl::not_null { std::move(s.value())->Accept(*this).value() }; |
There was a problem hiding this comment.
use std::move(s).value()
| #include "Oasis/Variable.hpp" | ||
| #include "Oasis/SimplifyVisitor.hpp" | ||
|
|
||
| inline Oasis::SimplifyVisitor simplifyVisitor{}; |
There was a problem hiding this comment.
inline shouldn't be necessary here; inline is only necessary for header files, (maybe you wanted static?). Or better yet, use an anonymous namespace https://stackoverflow.com/a/357464
| Oasis::Log{Oasis::EulerNumber{},Oasis::Multiply{Oasis::Real{6},Oasis::Variable{"x"}}}, Oasis::Variable{"x"}}; | ||
| auto diff = diffLog.Simplify(); | ||
| auto raw = diffLog.Accept(simplifyVisitor); | ||
| auto diff = std::move(raw.value()); |
There was a problem hiding this comment.
use std::move(raw).value()
2 participants
This is to leverage the visitor pattern and the ability to pass states with one object rather than passing a series of individual states through the simplify chain. An example of this would be degrees vs radians for trig functions. With the new visitor pattern, we can pass that option to all nodes that need it without passing additional parameters to each class's Simplify() function.