14

Background:

Consider the following example:

#include <iostream>
#include <vector>

int main() {
    std::vector<bool> vectorBool{false, true};
    for(const auto &element : vectorBool) std::cout << std::boolalpha << element << ' ';
    return 0;
}

It emits the warning:

test.cpp:6:21: warning: loop variable 'element' is always a copy because the range of type 'std::vector<bool>' does not return a reference [-Wrange-loop-analysis]
    for(const auto &element : vectorBool) std::cout << std::boolalpha << element << ' ';
                    ^
test.cpp:6:9: note: use non-reference type 'std::_Bit_reference'
    for(const auto &element : vectorBool) std::cout << std::boolalpha << element << ' ';
        ^~~~~~~~~~~~~~~~~~~~~
1 warning generated.

When compiled using clang with the range-loop-analysis diagnostic enabled:

$ clang++ -Wrange-loop-analysis -o test test.cpp

Questions:

According to https://reviews.llvm.org/D4169, the warning emits when:

for (const Foo &x : Foos), where the range Foos only return a copy. Suggest using the non-reference type so the copy is obvious

I completely understand that std::vector<bool>'s iterators return a copy of a proxy type (instead of a reference), but I disagree with the statement "so the copy is obvious":

  1. Where exactly is that "hidden" copy operation taking place? As far as I can see, we are just binding a reference to a temporary object, and this should prolong the lifetime of the temporary to match that of the reference.
  2. Even in case we have written for(const auto element : vectorBool) (so that the warning disappears), We should have no copy/move operations under C++17's guaranteed copy elision rules (and even in pre-C++17 when using any decent compiler), so is this warning about making an elided copy operation obvious?!
Community
  • 1
  • 1
Mike
  • 8,055
  • 1
  • 30
  • 44
  • At first glance this warning looks silly/wrong to me for the code you posted. – Jesper Juhl Apr 27 '18 at 15:58
  • @JesperJuhl It's unfortunate that proxy types gets this warning, but I doubt analyzers can ever distinguish between proxies and some weird stuff. If all else fails, blame `std::vector` – Passer By Apr 27 '18 at 15:59
  • 1
    @Passer By - `vector`, yeah, I wish it had never been created.. – Jesper Juhl Apr 27 '18 at 16:00
  • @PasserBy, so, do you mean that there are cases where dereferencing iterators return a copy (without this copy being a proxy object)? I have never encountered such a case before, and I can't imagine why would something like this be done – Mike Apr 27 '18 at 16:05
  • 1
    @Mike I don't want to see that ever being done either. The analysis only looks at the type and deduce that the return value cannot possibly be the element in the container, _something_ must have been copied. It then suggests writing `auto : Foos` to make obvious that fact. In this case, proxies conceptually didn't copy anything, but I doubt the analyzer can do anything about that. It's trying to warn people about the weird non-proxy case. – Passer By Apr 27 '18 at 16:08
  • 2
    @Mike Consider some sort of range adapter like `uppercase(some_vector_of_strings)`. It can't possibly return a reference to the object in the container since it has to modify each element before it returns it. – Miles Budnek Apr 27 '18 at 16:14
  • @MilesBudnek Actually, that worries me now that you mentioned it. That is exactly what ranges v3 is doing. Probably breaks the analysis extremely hard – Passer By Apr 27 '18 at 17:10
  • Here is an interesting series of articles regarding this topic: https://quuxplusone.github.io/blog/2018/12/15/autorefref-always-works/ Recommendation: always use `for ( auto&& element : container )` – Ignitor Dec 03 '20 at 15:09
  • @Ignitor Whenever you use a reference you are adding very subtle dependencies. If I do `for (auto const element:container)`, I **know** that the `element` is not mutated by any mutation of the container in the body of the for loop. While that is rare, when it happens it very often generates a bug. If the cost of copying the element is low, using reference aliases on it is borrowing trouble... – Yakk - Adam Nevraumont May 26 '21 at 14:43

1 Answers1

14

In C++17 a range based for loop is defined as

{
    auto && __range = range_expression ; 
    auto __begin = begin_expr ;
    auto __end = end_expr ;
    for ( ; __begin != __end; ++__begin) { 
        range_declaration = *__begin; 
        loop_statement 
    } 
}

And

range_declaration = *__begin;

Is the point where the range variable is initialized. Typically *__begin returns a reference so in

for (const auto& e : range_that_returns_references)

e can be eliminated and we can just work with the element from the range. In

for (const auto& e : range_that_returns_proxies_or_copies)

e can't be eleminated. *__begin will create a proxy or copy and then we bind that temporary to e. That means in every iteration you have an object that is being created and destroyed, which could be costly, and is not obvious as you are using a reference. The warning wants you to use a non reference type to make it obvious that you are not actually working with the element from the range but instead a copy/proxy of it.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
NathanOliver
  • 171,901
  • 28
  • 288
  • 402
  • 1
    I see, so the warning is just about making sure the user understands that there is a temporary object being created (and not just references being passed around). However, in my opinion, the warning should not be emitted when the user is using `for(auto && e : vectorBool)` since using forwarding references is enough to indicate that the user is willing to accept any kind of return type... – Mike Apr 27 '18 at 20:14
  • @Mike I think that would be reasonable. I'm not coming up with anything that where it would be non obvious. – NathanOliver Apr 27 '18 at 20:21