[Bug] unqualified auto on C++ container iterator

SonarQube Server / Community Build
,
1

Versions:

  • SonarQube 9.7.1 Enterprise
  • sonar-scanner 4.7.0.2747

I saw this code change in a PR. This was due to SonarQube complaining about the unqualified auto swallowing the pointer of the iterator. However the return type of begin() of our container is Container<T>::iterator with iterator defined as typedef T* iterator;.

-  for (auto it = container.begin(); it != container.end(); ++it) {
+  for (auto* it = container.begin(); it != container.end(); ++it) {

In terms of iterators this is a false positive. Since the typedef already contains that this type is a pointer. If we exchanged this container with a different container that uses a class as iterator, e.g. std::unordered_set this would break.

The analysis should differentiate between a pointer to a type and if there is a typedef/using declaration that already contains the pointer. A small reproduction shows that clang-tidy analyzes this correctly.

Best regards,

Carl

3

Hi @Carl,

To confirm, are you talking about this rule RSPEC?

This rule is disabled by default for the same argument that you made.

It is subjective:

  • The argument for it is usually readability/avoiding confusion.
  • The argument against it is what you mentioned about maintainability and being able to refactor the code without breaking it.

Many of us (the analyzer developers) tend to favor the second argument, and that is why it didn’t end up enabled by default.

As for exceptions, I don’t think it makes sense if you are a believer in the first argument. You can apply the argument you provided and say: if we exchange the pointer for a smart pointer, the code will break. Hence, the rule should never raise.

In short, the rule tells you when a raw pointer is swallowed by auto. If you enable it, it means that you want your code to break when the type is no longer a pointer.

Thanks,

4

Hi @Abbas,

yes, this is the rule we are using.

My argumentation is that this is not an exception. A typedef that contains a pointer is not the same as a pointer, since the auto isn’t “hiding” the fact that this is a pointer anymore, the typedef is.

Since iterators may or may not be pointers (e.g. for contiguous vs non-contiguous containers), this would mean different iteration code in both cases.

clang-tidy offers the same rule and seems to distinguish the two cases typedef including pointer, and pointer, which makes intuitive sense. Since in one case the auto is not swallowing.

Also the auto is deducing a typedef iterator, not a raw pointer, so we think this is wrong.

Best regards,

Carl

5

@Carl,

Thanks for the explanation, I think the argument of typedef swallowing the pointer instead of the auto makes more sense than the previous argument about breaking the code.

Since I’m against enabling this rule in general, I will flag this post to get a second opinion from the implementer.

Thanks for the feedback!

1 Like
8

@Carl,

First, a disclaimer: Given the choice, I would not enable this rule on my code, which means I might not be the best person to discuss it, but I will share my reasoning when the rule was put in place:

Why is showing the pointer so important? Because the fact that the type is a pointer matters, you don’t work with pointers the same way as you work with other types. You must care about reference semantics, you must ensure that the pointed-to object outlives its use through the pointer, and maybe you must care about deleting the pointed-to object.

All these concerns remain valid, no matter if the pointer is hidden by a typedef or not.

When using a typedef to a pointer, the fact that there is a pointer is usually not really hidden: The name of the typedef usually helps to remember that the code is dealing with a pointer (I’ve seen many typedefs with a Ptr suffix, for instance). I even suspect that codebases that require pointers to be explicit in auto cases would not accept a typedef to a pointer unless the typedef name clearly states what is happening (it’s however not easy for us to write a rule about that because it would require dabbling with the semantic of a name).

When using auto, there is no such possible help, therefore the pointer should remain visible for the declaration.

Therefore, I don’t think that, in general, ignoring a pointer contained within a typedef would be the right choice for this rule.

I’m however willing to consider the fact that this rule should not trigger if the “this is a pointer” information might be given by another name. For instance, in the following C++20 code:

for (std::forward_iterator auto  it = cont.begin(); it != cont.end() ; ++it) {

The “this is a pointer” is no longer hidden, because it is explicit that it is an iterator (for which the same kind of extra care must be taken as for pointers)

Do you think such a change would resolve your concern?

9

Since we don’t use C++20 this wouldn’t help us. I understand your reasoning, I still find the clang-tidy way to be better to only complain about raw pointers here. As you can see nobody enables this rule because it would complain in places like this. So if everybody disables it because it doesn’t care about typedefs it seems broken in terms of what the user actually wants to check.