astro-informatics / sopt

Sparse OPTimisation using state-of-the-art convex optimisation algorithms.
http://astro-informatics.github.io/sopt/
GNU General Public License v2.0
9 stars 11 forks source link

Modernize: use auto wherever possible for object initialisation #407

Open krishnakumarg1984 opened 1 year ago

krishnakumarg1984 commented 1 year ago

The C++ core guidelines, a living document co-authored by Stroustrup and Sutter, states in ES.11 that:

ES.11: Use auto to avoid redundant repetition of type names

Reason

  • Simple repetition is tedious and error-prone.
  • When you use auto, the name of the declared entity is in a fixed position in the declaration, increasing readability.
  • In a function template declaration the return type can be a member type.
Example

Consider:

auto p = v.begin();      // vector<DataRecord>::iterator
auto z1 = v[3];          // makes copy of DataRecord
auto& z2 = v[3];         // avoids copy
const auto& z3 = v[3];   // const and avoids copy
auto h = t.future();
auto q = make_unique<int[]>(s);
auto f = [](int x) { return x + 10; };

In each case, we save writing a longish, hard-to-remember type that the compiler already knows but a programmer could get wrong.

mmcleod89 commented 1 year ago

I personally disagree that auto makes code more readable. It can do, under some circumstances: as an example I think sometimes it makes for loops easier and cleaner where the type iterated over is obvious. In most code though, especially assignments, it can make it much easier for people who want to read and maintain the code to be able to clearly see correct type. In these cases, it is obvious because of the casts on the right hand side (but this also makes any errors obvious), but in your example code above it obfuscates the code. What is the type of h = t.future()? If I want to use this in my code and understand it, I now have to look up this function, instead of just reading right where it is declared. This spreading out of dependency makes existing code bases harder to get to grips with.

mmcleod89 commented 1 year ago

To be clear, I do think that auto can in principle help to prevent accidental implicit casts (which I assume are the errors referrered to when they describe manual typing as "error prone"), but I don't think this is a common problem in practice, and I disagree with the conclusion to use auto "wherever possible". For the changes made here I think it's fine because it's always clear what the type is anyway, but it also doesn't really make much of a difference: it doesn't correct any code that was wrong and I don't think that the resulting code is any more readable than before.

krishnakumarg1984 commented 1 year ago

@mmcleod89, at least for things like:

Eigen::ArrayBase<T0> &coeffs = const_cast<Eigen::ArrayBase<T0> &>(coeffs_);
Eigen::ArrayBase<T1> &signal = const_cast<Eigen::ArrayBase<T1> &>(signal_);

we could strongly consider auto to avoid the redundancy with long data types. The following is more readable in my opinion.

auto &coeffs = const_cast<Eigen::ArrayBase<T0> &>(coeffs_);
auto &signal = const_cast<Eigen::ArrayBase<T1> &>(signal_);