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

Clang tidy cleanup and using std algorithms #1373

Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Clang tidy with modernize-use-equals-default,readability-braces-aroun…
…d-statements,performance-unnecessary-value-param
  • Loading branch information
SteveBronder committed Sep 30, 2019
commit b9f545b8ab05b268e04cbd0d57b5a0005355671a
2 changes: 1 addition & 1 deletion stan/math/prim/mat/fun/accumulator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class accumulator {
/**
* Destroy an accumulator.
*/
~accumulator() {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a reason for defining the accumulator destructor as empty here? tmk this still calls the destructor for all the accumulates members

Copy link
Contributor

Choose a reason for hiding this comment

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

This one is OK to leave as default---as is, it's not virtual and breaks the rule of 3(5).

~accumulator() = default;

/**
* Add the specified arithmetic type value to the buffer after
Expand Down
2 changes: 1 addition & 1 deletion stan/math/prim/mat/fun/matrix_exp_action_handler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class matrix_exp_action_handler {
public:
/* Constructor
*/
matrix_exp_action_handler() {}
matrix_exp_action_handler() = default;

/* Perform the matrix exponential action exp(A*t)*B
* @param [in] mat matrix A
Expand Down
2 changes: 1 addition & 1 deletion stan/math/prim/mat/meta/broadcast_array.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace internal {
template <typename ViewElt, typename OpElt, int R, int C>
class empty_broadcast_array<ViewElt, Eigen::Matrix<OpElt, R, C> > {
public:
empty_broadcast_array() {}
empty_broadcast_array() = default;
/**
* Not implemented so cannot be called.
*/
Expand Down
6 changes: 3 additions & 3 deletions stan/math/prim/mat/meta/operands_and_partials.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class ops_partials_edge<ViewElt, Eigen::Matrix<Op, R, C>> {
using partials_t = empty_broadcast_array<ViewElt, Eigen::Matrix<Op, R, C>>;
partials_t partials_;
empty_broadcast_array<partials_t, Eigen::Matrix<Op, R, C>> partials_vec_;
ops_partials_edge() {}
ops_partials_edge() = default;
explicit ops_partials_edge(const Eigen::Matrix<Op, R, C>& /* ops */) {}

private:
Expand All @@ -38,7 +38,7 @@ class ops_partials_edge<ViewElt, std::vector<Eigen::Matrix<Op, R, C>>> {
public:
using partials_t = empty_broadcast_array<ViewElt, Eigen::Matrix<Op, R, C>>;
empty_broadcast_array<partials_t, Eigen::Matrix<Op, R, C>> partials_vec_;
ops_partials_edge() {}
ops_partials_edge() = default;
explicit ops_partials_edge(
const std::vector<Eigen::Matrix<Op, R, C>>& /* ops */) {}

Expand All @@ -59,7 +59,7 @@ class ops_partials_edge<ViewElt, std::vector<std::vector<Op>>> {
= empty_broadcast_array<ViewElt, std::vector<std::vector<Op>>>;
partials_t partials_;
empty_broadcast_array<partials_t, std::vector<std::vector<Op>>> partials_vec_;
ops_partials_edge() {}
ops_partials_edge() = default;
explicit ops_partials_edge(const std::vector<std::vector<Op>>& /* ops */) {}

private:
Expand Down
7 changes: 5 additions & 2 deletions stan/math/prim/mat/prob/ordered_logistic_glm_lpmf.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,14 @@ ordered_logistic_glm_lpmf(
check_finite(function, "First cut-point", cuts[0]);
}

if (size_zero(y, cuts))
if (size_zero(y, cuts)) {
return 0;
}

if (!include_summand<propto, T_x_scalar, T_beta_scalar, T_cuts_scalar>::value)
if (!include_summand<propto, T_x_scalar, T_beta_scalar,
T_cuts_scalar>::value) {
return 0;
}

const auto& x_val = value_of_rec(x);
const auto& beta_val = value_of_rec(beta);
Expand Down
2 changes: 1 addition & 1 deletion stan/math/prim/scal/meta/broadcast_array.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class broadcast_array {
template <typename T, typename S>
class empty_broadcast_array {
public:
empty_broadcast_array() {}
empty_broadcast_array() = default;
/**
* Not implemented so cannot be called.
*/
Expand Down
2 changes: 1 addition & 1 deletion stan/math/prim/scal/meta/operands_and_partials.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class ops_partials_edge {
public:
empty_broadcast_array<ViewElt, Op> partials_;

ops_partials_edge() {}
ops_partials_edge() = default;
explicit ops_partials_edge(const Op& /* op */) {}

private:
Expand Down
2 changes: 1 addition & 1 deletion stan/math/rev/core/chainable_alloc.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class chainable_alloc {
chainable_alloc() {
ChainableStack::instance_->var_alloc_stack_.push_back(this);
}
virtual ~chainable_alloc() {}
virtual ~chainable_alloc() = default;
};

} // namespace math
Expand Down
2 changes: 1 addition & 1 deletion stan/math/rev/core/gevv_vvv_vari.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class gevv_vvv_vari : public vari {
v2_[i] = v2[i * stride2].vi_;
}
}
virtual ~gevv_vvv_vari() {}
virtual ~gevv_vvv_vari() = default;
void chain() {
const double adj_alpha = adj_ * alpha_->val_;
for (size_t i = 0; i < length_; i++) {
Expand Down
2 changes: 1 addition & 1 deletion stan/math/rev/mat/fun/mdivide_left_ldlt.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace internal {
template <int R1, int C1, int R2, int C2>
class mdivide_left_ldlt_alloc : public chainable_alloc {
public:
virtual ~mdivide_left_ldlt_alloc() {}
virtual ~mdivide_left_ldlt_alloc() = default;

/**
* This share_ptr is used to prevent copying the LDLT factorizations
Expand Down
2 changes: 1 addition & 1 deletion stan/math/rev/mat/fun/mdivide_left_spd.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace internal {
template <int R1, int C1, int R2, int C2>
class mdivide_left_spd_alloc : public chainable_alloc {
public:
virtual ~mdivide_left_spd_alloc() {}
virtual ~mdivide_left_spd_alloc() = default;

Eigen::LLT<Eigen::Matrix<double, R1, C1> > llt_;
Eigen::Matrix<double, R2, C2> C_;
Expand Down
3 changes: 2 additions & 1 deletion stan/math/rev/mat/functor/algebra_solver_newton.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,9 @@ Eigen::Matrix<T2, Eigen::Dynamic, 1> algebra_solver_newton(
dat_int, theta_dbl, fx, msgs);
Eigen::Matrix<var, Eigen::Dynamic, 1> theta(x.size());
theta(0) = var(vi0->theta_[0]);
for (int i = 1; i < x.size(); ++i)
for (int i = 1; i < x.size(); ++i) {
theta(i) = var(vi0->theta_[i]);
}

return theta;
}
Expand Down
10 changes: 6 additions & 4 deletions stan/math/rev/mat/functor/algebra_system.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ struct system_functor {
/** stream message */
std::ostream* msgs_;

system_functor() {}
system_functor() = default;

system_functor(const F& f, const Eigen::Matrix<T0, Eigen::Dynamic, 1>& x,
const Eigen::Matrix<T1, Eigen::Dynamic, 1>& y,
Expand Down Expand Up @@ -161,18 +161,20 @@ struct hybrj_functor_solver : nlo_functor<double> {

template <typename T>
void algebra_solver_check(const Eigen::Matrix<T, Eigen::Dynamic, 1>& x,
const Eigen::VectorXd y,
const Eigen::VectorXd& y,
const std::vector<double>& dat,
const std::vector<int>& dat_int,
double function_tolerance,
long int max_num_steps) { // NOLINT(runtime/int)
check_nonzero_size("algebra_solver", "initial guess", x);
check_finite("algebra_solver", "initial guess", x);
check_finite("algebra_solver", "parameter vector", y);
for (double i : dat)
for (double i : dat) {
check_finite("algebra_solver", "continuous data", i);
for (int x : dat_int)
}
for (int x : dat_int) {
check_finite("algebra_solver", "integer data", x);
}

check_nonnegative("algebra_solver", "function_tolerance", function_tolerance);
check_positive("algebra_solver", "max_num_steps", max_num_steps);
Expand Down
2 changes: 1 addition & 1 deletion stan/math/rev/mat/functor/cvodes_integrator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ namespace math {
template <int Lmm>
class cvodes_integrator {
public:
cvodes_integrator() {}
cvodes_integrator() = default;

/**
* Return the solutions for the specified system of ordinary
Expand Down
6 changes: 4 additions & 2 deletions stan/math/rev/mat/functor/kinsol_data.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,11 @@ struct kinsol_J_f {
Eigen::MatrixXd Jac;
jacobian(system, to_vector(x_vec), fx, Jac);

for (int i = 0; i < Jac.rows(); i++)
for (int j = 0; j < Jac.cols(); j++)
for (int i = 0; i < Jac.rows(); i++) {
for (int j = 0; j < Jac.cols(); j++) {
SM_ELEMENT_D(J, i, j) = Jac(i, j);
}
}

return 0;
}
Expand Down
9 changes: 6 additions & 3 deletions stan/math/rev/mat/functor/kinsol_solve.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,22 +105,25 @@ Eigen::VectorXd kinsol_solve(
kinsol_data.LS_, kinsol_data.J_),
"KINSetLinearSolver");

if (custom_jacobian)
if (custom_jacobian) {
check_flag_sundials(
KINSetJacFn(kinsol_data.kinsol_memory_, &system_data::kinsol_jacobian),
"KINSetJacFn");
}

N_Vector nv_x = N_VNew_Serial(N);
for (int i = 0; i < N; i++)
for (int i = 0; i < N; i++) {
NV_Ith_S(nv_x, i) = x(i);
}

check_flag_kinsol(KINSol(kinsol_data.kinsol_memory_, nv_x, global_line_search,
scaling, scaling),
max_num_steps);

Eigen::VectorXd x_solution(N);
for (int i = 0; i < N; i++)
for (int i = 0; i < N; i++) {
x_solution(i) = NV_Ith_S(nv_x, i);
}

N_VDestroy(nv_x);
N_VDestroy(scaling);
Expand Down