New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Misc cleanup -- Get rid of ExprEvalInterface #870
Conversation
A new Pull Request was created by @ktf (Giulio Eulisse) for CMSSW_7_0_X. Misc cleanup -- Get rid of ExprEvalInterface It involves the following packages: DetectorDescription/Core @civanch, @Dr15Jones, @ianna, @mdhildreth, @nclopezo, @ktf can you please review it and eventually sign? Thanks. |
Pull request #870 was updated. @civanch, @Dr15Jones, @ianna, @mdhildreth, @nclopezo, @ktf can you please check and sign again. |
Pull request #870 was updated. @civanch, @Dr15Jones, @ianna, @mdhildreth, @nclopezo, @ktf can you please check and sign again. |
@@ -63,6 +64,7 @@ class DDLElementRegistry //: public DDXMLElementRegistry | |||
|
|||
/// Get the name given a pointer. This may not be needed... | |||
const std::string& getElementName(DDXMLElement* theElement) const; | |||
ClhepEvaluator &evaluator() { return ExprEval::instance(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ktf I wonder why would you like to do this instead of using ExprEval::instance() directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this way all the accesses to ExprEvalEvaluator go through a instance method of the DDLElementRegistry which is already percolated to the deepest level it is needed (the various DDXMLElements or whatever they are called). This allows me to remove all the references to ExprEvalEvaluator::instance
apart from this one and making it just a property of DDLElementRegistry will do the trick of getting rid of the ExprEvalEvaluator singleton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me.
I run the following test: The before and after results are:
|
+1 |
This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it? |
As discussed with @ianna the variance on the measurement is actually larger than the difference reported. Approving this for the time being. We'll keep it in mind if we see a big regression WRT pre4 at the end of the clean up. All the produced files are equal. |
Misc cleanup -- Get rid of ExprEvalInterface
Discussed with @ianna over coffee. Given we have only one evaluator we can simply get rid of the abstract base class and clean up the code.