Better for loops, part 3/N

[kunaltyagi]

No description provided.

[jasjuang]

I feel like a lot of changes in this PR decreases the readability of the code. I can't tell what the types are by looking at the code anymore. For example,

auto n_neighbours = this->searchForNeighbors (index, search_parameter_, neighbours_indices, neighbours_distances);

Can't tell whether n_neighbours is int, uint, or even int64_t. If there's a bug in searchForNeighbors that makes it return float, the compiler won't raise any warnings anymore.

So what are the main benefits of making these changes?

[kunaltyagi]

I feel like a lot of changes in this PR decreases the readability of the code

Sure. Please point them out and I'll remove them. That wasn't my intent.

Can't tell whether n_neighbours is int, uint, or even int64_t

I'll use std::size_t in my modification. I can see your point of view and will change my code to be more explicit.

Just to present my side: I'm of the opinion that despite C++ being a strongly typed language, most of the times, we shouldn't need to care about the return type. The language ensures that there's no overloading on return type. Code is a communication with the future readers*. Presence of the type implies that the person needed the type to be that, and couldn't trust the compiler; absence implies the compiler can choose the best type for this situation. Most of the times, we just want to work with the return type, so auto is a good fit. Maybe working with Haskell (and Python and Erlang) has made me not like overt types.

*: This actually goes both ways. My code needs to be readable by the maintainers too, and I need to adapt to the community.

[jasjuang]

I think auto here decreases readability for the same reasons.

[kunaltyagi]

auto here enables changing the indices to std::size_t or std::ptrdiff_t later. That change will be quite difficult without minor changes like this.

[jasjuang]

@kunaltyagi That's a valid point. But I would argue that the change won't be that difficult with vim + regular expression search and replace. So I would rather prefer keeping good readability.

[jasjuang]

Personally I am not a big fan of auto. I read the Herb Sutter article you linked but I am still not convinced. Of course, I can't compare myself with Herb but here are my thoughts. I think using auto only makes sense in the below scenario.

const boost::this::type::is::way::too::long a = static_cast<boost::this::type::is::way::too::long>(b);

this is way too long so if we do the below,

const auto a = static_cast<boost::this::type::is::way::too::long>(b);

it shortens the code and I still know what type a is by looking at the code from the first glance. This is a very rare scenario because most of the time boost::this::type::is::way::too::long will be used all over the place so it's better to typedef it with something that's shorter and meaningful.

I think using auto all over the place is a bad idea, it decreases readability and a lot of people down at the comment section of the Herb Sutter article agree with me too.

[kunaltyagi]

It's an opinion, and a contentious one. I've modified the patch to remove that. I'll check other patches too 😄

[kunaltyagi]

108 is a flaky test. Passes based on a timeout. Reported earlier too.

[taketwo]

I find myself on the auto side of the debate. I believe auto not only makes code more compact (by shortening lengthy type names), but also more expressive. As pointed by @kunaltyagi, usually we intend to work with the return type (whatever that might be) without conversions, and auto is the means to express just that.