Modify rule S5500: mention std::ranges::move and rewrite RSPEC (CPP-5219)

[marco-antognini-sonarsource]

Companion PR: SonarSource/sonar-cpp#3622

[marco-antognini-sonarsource]

validate_links fails for unrelated reasons.

[loic-joly-sonarsource]

I'll let you decide if you want to apply the suggestion

[loic-joly-sonarsource]

I like the rewrite a lot.

[loic-joly-sonarsource]

Suggested change

	store.emplace_back(shape); // Noncompliant, call to std::move is expected.
	store.emplace_back(shape); // Noncompliant, calls Shape's copy constructor

[marco-antognini-sonarsource]

I wanted to highlight in the code snippet the root cause of the issue as generically as possible.

For example, this would be compliant:

if (shape.isVisible()) { store.emplace_back(shape); }
foo(std::move(shape));

because std::move is called.

In a way, we don't care about the copy/move constructors.

The sentence below is meant to explain what is actually happening and justify why std::move is required. It also serves to connect the two examples.

Does that make sense?

[loic-joly-sonarsource]

In that case, what would you think of:

void DrawingStore::insertVisibleShape(Shape&& shape) { // Noncompliant, Shape is never moved
  if (shape.isVisible()) {
    store.emplace_back(shape); // Calls Shape's copy constructor.
  }
}

Which would be corrected as:

void DrawingStore::insertVisibleShape(Shape&& shape) { // Compliant
  if (shape.isVisible()) {
    store.emplace_back(std::move(shape)); // Calls Shape's move constructor.
  }
}

To me, the benefits are:

• As you mention, even if a call to std::move is expected, it could be called in other places
• I think it more clearly describes what actually happens in the code because:
  • Inline comments avoid having to refer to a place in the code in the next sentence
  • There is a symmetry in the inline comments that highlight the difference

If you still disagree, I'm not going to fight. The RSPEC is correct as it is. I just feel it could be improved.

[marco-antognini-sonarsource]

Your idea is not bad, but it would take me a bit of time to adapt and make other sections consistent.

It also has a downside: the diff highlighting will include the first line of the (non)compliant code because of the comment, but only the comment changes. Since the diff is on the whole line (and not on words), this will distract the readers a bit.

So I'll keep the current wording for now. We can always revisit this in the future.

[loic-joly-sonarsource]

I'm still suggesting changes, but feel free to discard them if you disagree with them.

[loic-joly-sonarsource]

In that case, what would you think of:

void DrawingStore::insertVisibleShape(Shape&& shape) { // Noncompliant, Shape is never moved
  if (shape.isVisible()) {
    store.emplace_back(shape); // Calls Shape's copy constructor.
  }
}

Which would be corrected as:

void DrawingStore::insertVisibleShape(Shape&& shape) { // Compliant
  if (shape.isVisible()) {
    store.emplace_back(std::move(shape)); // Calls Shape's move constructor.
  }
}

To me, the benefits are:

• As you mention, even if a call to std::move is expected, it could be called in other places
• I think it more clearly describes what actually happens in the code because:
  • Inline comments avoid having to refer to a place in the code in the next sentence
  • There is a symmetry in the inline comments that highlight the difference

If you still disagree, I'm not going to fight. The RSPEC is correct as it is. I just feel it could be improved.

[sonarqube-next]

Quality Gate passed for 'rspec-frontend'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

[sonarqube-next]

Quality Gate passed for 'rspec-tools'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube