Skip to content
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

DM-29501: replace unary_function, binary_function, deprecated in C++17 #576

Merged
merged 1 commit into from Apr 2, 2021

Conversation

mwittgen
Copy link
Contributor

No description provided.

Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the changes are good so far, but I fear that they are incomplete, in particular regarding Integrate.h and friends. I can't comment on the non-diff code, so I have links here.
The basic problem is that there are two different interrelated code-paths here. First, we have integrate which is a front-end for int1d; integrate2d which does a double loop over integrate. As best as I can tell, this is the code path that is used in the stack and all the tests (except for the one example that has been commented out).
Second, we have int2d and int3d which (a) aren't used; (b) aren't tested; (c) aren't updated. (In particular,

/**
* Helpers for constant regions for int2d, int3d:
*
*/
template <class T>
struct ConstantReg1 : public std::unary_function<T, IntRegion<T> > {
ConstantReg1(T a, T b) : ir(a, b) {}
ConstantReg1(IntRegion<T> const &r) : ir(r) {}
IntRegion<T> operator()(T) const { return ir; }
IntRegion<T> ir;
};
template <class T>
struct ConstantReg2 : public std::binary_function<T, T, IntRegion<T> > {
ConstantReg2(T a, T b) : ir(a, b) {}
ConstantReg2(IntRegion<T> const &r) : ir(r) {}
IntRegion<T> operator()(T x, T y) const { return ir; }
IntRegion<T> ir;
};
are not going to compile with c++17 but aren't used so don't fail tests).
Relatedly, there is code comment documentation
// Two- and Three-Dimensional Integrals:
//
// These are slightly more complicated. The easiest case is when the
// bounds of the integral are a rectangle or 3d box. In this case,
// you can still use the regular IntRegion. The only new thing then
// is the definition of the function. For example, to integrate
// int(3x^2 + xy + y , x=0..1, y=0..1):
//
// struct Integrand :
// public std::binary_function<double,double,double>
// {
// double operator()(double x, double y) const
// { return x*(3.*x + y) + y; }
// };
//
// integ::IntRegion<double> reg3(0.,1.);
// double integ3 = int2d(Integrand(),reg3,reg3);
//
// (Which should give 1.75 as the result.)
//
//
//
that is now out of date, and furthermore refers to the code path that isn't actually used so has been out of date for some untold years.

My thought would be to remove the incorrect documentation and the unused, untested, and probably broken anyway int2d and int3d code paths and friends (but of course leave int1d which is used by integrate and integrate2d).

tests/test_integrate.cc Outdated Show resolved Hide resolved
@mwittgen mwittgen force-pushed the tickets/DM-29501 branch 2 times, most recently from 31df64c to c162694 Compare April 1, 2021 21:56
Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, with all the dead code that I know of now removed. Thanks!
All good assuming that this compiles and tests pass with both C++14 and C++17.

@mwittgen mwittgen merged commit 869f10c into master Apr 2, 2021
@mwittgen mwittgen deleted the tickets/DM-29501 branch April 2, 2021 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants