Question about the new return a reference check?

[Steve Albright]

I'm getting pushback related to changing to using a const reference instead of letting it do a copy.

The concern is that in a multi-threaded app that if we return a const reference to a collection, could something else change the collection and affect the const reference that was returned?

Basically, are there scenarios where we don't want to change to a const reference and if so, are all accounted for in this new check?

[Oliver Stöneberg]

I understand the concern that's why the check was done to only report it in a very narrow case (see https://trac.cppcheck.net/ticket/12546).

If you just change the class there should be no behavior in the code at all since you should still store it in anon-reference and thus using a copy.

Unless you had the pattern that you define the local variable as a temporary const reference although it does return a copy. That is a pattern I would discourage though (would make sense to report this an issue - will file a ticket later if none exists yet).

If you are really planning to store references to data within objects that data should not be mutable within the object and the object instance you referencing should also not be mutable. That just seems like a cause for trouble to begin with.

[Daniel Marjamäki]

hmm I think that is interesting feedback. I guess fixing this could break the code. Even in non-threaded cases.

[Daniel Marjamäki]

For example, if a getter is called like this:

const auto& start = fred.getX();
fred.dostuff();
if (start == fred.getX()) {}

It's unfortunate to suggest that getX returns a const reference here.

[Oliver Stöneberg]

That looks like flawed code. If fred is const there should be no issue as this code would not compile. In case you are taking a reference from a mutable object you should not store a reference but a copy.

[Daniel Marjamäki]

It's not very flawed imho. fred is not const obviously. The scenario is that it compiles.

[Daniel Marjamäki]

how flawed do you think this is.. would you even think that a warning would be wanted if calling a method to a non-const object and storing the result in a const reference?

[Daniel Marjamäki]

About QString. If we add attribute view=true to the QString configuration I don't think Cppcheck will warn about it. I don't know off the top of my head for sure if that is proper.

[Daniel Marjamäki]

The concern is that in a multi-threaded app that if we return a const reference to a collection, could something else change the collection and affect the const reference that was returned?

I think it would be interesting with a minimal example code where Cppcheck writes a false positive.

[Steve Albright]

Example code that doesn't produce the warning but shows how things could change (basically your suggestion above). So the code isn't really flawed and does compile.

class ConstRefExample
{
public:
   ConstRefExample()
      : TheList{1,2,3,4,5}
   {
   }

   const QList<int>& GetIntegerList() const
   {
      return TheList;
   }

   void RemoveListItem(int itemToRemove)
   {
      TheList.removeOne(itemToRemove);
   }

private:
   QList<int> TheList;
};

TEST(TripUpConstReferenceReturn, DoesItHappen)
{
   ConstRefExample example;
   const QList<int>& list = example.GetIntegerList();
   const QList<int> list2 = example.GetIntegerList();
   QList<int> list3 = example.GetIntegerList();
   auto list4 = example.GetIntegerList();
   auto& list5 = example.GetIntegerList();
   const auto& list6 = example.GetIntegerList();

   EXPECT_TRUE(list.contains(3));
   EXPECT_TRUE(list2.contains(3));
   EXPECT_TRUE(list3.contains(3));
   EXPECT_TRUE(list4.contains(3));
   EXPECT_TRUE(list5.contains(3));
   EXPECT_TRUE(list6.contains(3));

   example.RemoveListItem(3);      // simulate another thread changing the container
   EXPECT_TRUE(list.contains(3));  // fails - const reference
   EXPECT_TRUE(list2.contains(3)); // passes
   EXPECT_TRUE(list3.contains(3)); // passes
   EXPECT_TRUE(list4.contains(3)); // passes
   EXPECT_TRUE(list5.contains(3)); // fails - auto&
   EXPECT_TRUE(list6.contains(3)); // fails - const auto&
}

[lonnie563563]

Certainly! When considering the new return on a reference check, it’s essential to assess how the insights align with the candidate’s experience. For instance, if a reference highlights the candidate’s work on outdoor projects, including their management of a balcony turf installation, it could provide valuable context about their practical skills and suitability for the role. Understanding these details helps ensure a more informed hiring decision.