Transform classic loops to range-based for loops in module common

[SunBlack]

Changes are based on the result of run-clang-tidy -header-filter='.' -checks='-,modernize-loop-convert' -fix

[taketwo]

In this pull request some loops take auto variable by reference, and some by value. In the latter cases it means a useless copy. Of course, one may argue that for int or float this does not matter. True, but for complex types there is a performance penalty. In my opinion, is easier to just stick with the following rule of thumb: use const auto& in range-for loops by default, drop const if need to modify the contents. This way the code is more uniform and we don't need to think whether the type in question is complex or primitive.

[SunBlack]

In case I missed sth., tell me.

[SunBlack]

@taketwo reference on a primitive has no benefit, thats why I don't use it there. Furthermore I don't use auto for primitive data types. This makes even sense if we wan't to respect this: #2731 (comment) (MinTypeNameLength => so int and other simple data types will be not replaced)

[taketwo]

reference on a primitive has no benefit

This is true. But it also does not have deficit.

I don't use auto for primitive data types

Why? For me, the most important thing about auto is not that we save time on typing. By using auto we avoid unnecessary/accidental type casts! We also make our code more future-proof. If the type of the elements in the container changes in future, we don't need to adjust types in every loop.

So const auto& is a double-win IMHO. Guaranteed no type casts, guaranteed no copies. And no need to think and/or even check what is the type of elements in the container.

@SergioRAgostinho thoughts?

[SunBlack]

reference on a primitive has no benefit

This is true. But it also does not have deficit.

It has: I have to adjust automated adjusted code of clang-tidy ;)

But for real, there is a small difference for the CPU:

• for (int a : container): A value of container will be copied to a and can be used direct
• for (int &a : container): A reference to value of container will be stored into a. If a will be now used, the CPU has to look into a and follow the pointer to real storage pointer (Indirect addressing). This should not make any performance difference, because CPUs are optimized for it, but nevertheless: Why make it more complex, if it is not necessary?

I don't use auto for primitive data types

Why? For me, the most important thing about auto is not that we save time on typing. By using auto we avoid unnecessary/accidental type casts! We also make our code more future-proof. If the type of the elements in the container changes in future, we don't need to adjust types in every loop.

So const auto& is a double-win IMHO. Guaranteed no type casts, guaranteed no copies. And no need to think and/or even check what is the type of elements in the container.

True and wrong. auto is perfect for template methods. But in general using auto decreases readability in some cases. For complex data types it's fine, because e.g. if you see auto pointcloud = ... instead of PointCloud pointcloud = the data type is mostly clear. But if you see auto index = ..., do you know if index is e.g. signed or unsigned (important sometimes, because -1 is often used as invalid value, so it is good to know if there could be such an invalid value defined)? So in case of primitive datatypes it is hard to code for everyone, who do not use an IDE, becasue they have no idea about the data type (e.g. me for code I can't compile with MSVC and just in my VM ;) ). So yeah, maybe you prevent some accidental cast, but for this issue you have compiler warnings. But you decrease readability a lot, especially now you know: If you see auto it is mostly not a primitve datatype (except in rare template cases). So imho it is not worth to spam auto

[SergioRAgostinho]

I am more strict than both of you and usually just use auto for iterators and lambda types because I don't like to not know which type I'm looking at in most cases. I would prefer to transition very conservatively to auto and only use in these two uses cases I mentioned.

That being said, @SunBlack you're not being consistent between what you're saying and the code you're submitting (I believe). #2837 was mostly conversions over primitive types and they all got converted to auto. BS on my side. Most uses were normal. There were only two slips

I'm just getting concerned over the lack of consistency :/

[SunBlack]

That being said, @SunBlack you're not being consistent between what you're saying and the code you're submitting (I believe).

This decision is done by clang-tidy automatically. I just add a lot of const modifier and sometimes rename variables. Nevertheless clang-tidy isn't always consistent (sometimes for (int & i : container) and sometimes for (int i : container), even content of loop is equal).

#2837 was mostly conversions over primitive types and they all got converted to auto.

There was only one loop and this was a loop over std::vector<std::vector<int>>, so range declaration was std::vector<int> and not int ;)

[taketwo]

@SunBlack thanks for the explanation. I understand your points but disagree.

But if you see auto index = ..., do you know if index is e.g. signed or unsigned (important sometimes, because -1 is often used as invalid value, so it is good to know if there could be such an invalid value defined)?

Well, let's have a Gedankenexperiment here. Suppose you read code and see a loop

for (const int& index : indices)

Questions: 1) what is the type of indices? 2) should you expect/check for invalid (-1) indices?

My claim is that no, you can not be sure that the type of indices is int. It can easily be a vector of unsigned int. But even if the type is indeed int, you can not know whether -1 indices are used without consulting documentation (or reading codebase). Just for the record, there is no convention of invalid -1 indices in PCL even though indices are typically represented by int.

So yeah, maybe you prevent some accidental cast, but for this issue you have compiler warnings.

Not necessarily, there may be a legal non-narrowing conversion from the actual type to the type you used in the loop. In this case compiler will not complain.

But you decrease readability a lot,

I don't think so. By using auto you simply say: use exactly the type of the container, whatever that type is. How is it less readable than specifying some concrete type (which may even be wrong)? In fact, in many (most) cases you should view the actual type is an implementation detail which may change.

Scott Meyers in his "Effective Modern C++" recommends to prefer auto to explicit type declarations whenever possible (Item 5). I think we should stick with his advice.

Finally, regarding the references:

This should not make any performance difference, because CPUs are optimized for it, but nevertheless: Why make it more complex, if it is not necessary?

Depends on the viewpoint. Compiler will do the right thing anyway, and for me it less complex to use reference always (no-brainer!), compared to thinking every time whether the type is primitive or not.

[jasjuang]

I think it's a good idea to replace index based or iterator based for loops into range based for loops, but I don't think it's a good idea to use auto simply because there isn't an easy way to figure out immediately what the type is that we are looping through.

For implicit type casts it should be avoided anyways, otherwise we are asking for bugs for ourselves, I would argue that auto potentially makes it harder to discover the problem.

I also think if it's a primitive type that we are looping through it should by value, for non primitive type it should be by reference.

[taketwo]

Thanks for joining the discussion.

I also think if it's a primitive type that we are looping through it should by value, for non primitive type it should be by reference.

Could you give any pragmatic reason for this distinction? Also, let's clarify the language here, what exactly is "primitive type". Is this a built-in type (e.g. float, double)? Or is this a type with a small size (if so, how small)? For example, should std::pair<int, int> be counted as primitive and taken by value? What about struct Foobar { int a; }?

[jasjuang]

Yes I am referring to built-in types like ‘int’, ‘float’. To be honest this principle is based on copying built-in types are cheaper than indirect addressing and oppositely for non built-in types. However I understand that this principle is probably not applicable anymore for modern compilers so in the end it comes down to personal preferences. If we want to do const reference for all types in range for loops, for consistency purposes, should we also replace all pass by values in functions to pass by const references?

[taketwo]

should we also replace all pass by values in functions to pass by const references?

That would be a rather big endeavor with no apparent benefits. Besides, it will break user code that derives from our classes and overrides affected functions. So this is out of question IMHO.

As for the for loops, we are now going through all of them, so consistently using const references in all of them is possible. Unless someone formulates a clear rule which types should be by value and which by reference, and provides a justification why this distinction makes a difference, I would insist on going with "const reference always" rule.

Speaking of auto, I still believe "always-auto" is clear and simple rule that yields more maintainable code. However, since there is a strong opposition to this, I guess I will have to accept the majority opinion here. Let's avoid any explicit rules about typing, let code authors (and refactorers) decide what to use.

[SergioRAgostinho]

Let's avoid any explicit rules about typing, let code authors (and refactorers) decide what to use.

Just to be clear, the idea is: free for all. Not even imposing auto on iterators? I have the impression we all agreed on iterators without auto being evil.

[taketwo]

It's hard for me to imagine someone will contribute code with non-auto iterators anyway 😆

If you guys can agree on a list of cases where auto is compulsory, I'm all pro formalizing this to a rule. (As I said, for me all non-auto are evil, so however your list end up looking, it will be a subset of my hypothetical list).

[taketwo]

I haven't reviewed all of the transition PRs, but I've seen enough inconsistent usage of references, const-copies, and non-const copies, when the purpose of the loop is to view the contents of the container. Thus I wanted to reiterate on my proposal regarding references. I insist that by default we always use const references no matter the type. If the items are to be modified, then the const qualifier is dropped. In the (extremely rare) case when a copy is needed there should be no reference, of course.

Please do comment if there are any concerns or disagreement with this.

[SergioRAgostinho]

I already let a couple slip through under the radar then :/ My apologies. I'll enforce the proposal from now on.

[SunBlack]

I'm currently in vacation. But maybe you can already update style guide with all rules when use auto, const reference, ... (in loops and in general)