[clang-tidy] readability-redundant-member-init: flag assignment with std::nullopt

[chrchr-github]

#include <optional>

struct S {
    std::optional<int> o1 = {};           // warns here, OK 
    std::optional<int> o2 = std::nullopt; // should also warn here, but it doesn't
};

<source>:4:27: warning: initializer for member 'o1' is redundant [readability-redundant-member-init]
    4 |     std::optional<int> o1 = {};
      |

https://godbolt.org/z/jzTcf5rs1

Related: #43786

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: None (chrchr-github)

~~~c++ #include <optional>

struct S {
std::optional<int> o1 = {};
std::optional<int> o2 = std::nullopt; // warn here
};

<source>:4:27: warning: initializer for member 'o1' is redundant [readability-redundant-member-init]
4 | std::optional<int> o1 = {};
|

https://godbolt.org/z/jzTcf5rs1
</details>

[carlosgalvezp]

If I understand correctly, you are reporting a False Negative, i.e. you'd want clang-tidy to warn about the constructor that sets the std::nullopt.

I believe the check is working according to the documentation. The check is intended to catch cases where the default constructor would do the same thing as the initializer:

Finds member initializations that are unnecessary because the same default constructor would be called if they were not present.

However, assigning to std::nullopt, you are not calling the default constructor of std::optional, but instead calling the constructor std::optional(std::nullopt_t).

Supporting this use case would mean that:

• The check must have specific logic for just std::optional.
• The check must know that the constructor std::optional(std::nullptr_t) is exactly equivalent to the default constructor.

Personally I believe checks should be generic and not have ad-hoc logic for such narrow use cases, since it does not scale and greatly increases maintenance cost. But it would be good to hear from others what they think.

[chrchr-github]

If I understand correctly, you are reporting a False Negative, i.e. you'd want clang-tidy to warn about the constructor that sets the std::nullopt.

Correct.

I believe the check is working according to the documentation. The check is intended to catch cases where the default constructor would do the same thing as the initializer:

I think this argument then also applies to #43786 and possibly other cases.
Re ad-hoc logic: IMHO the STL deserves special-casing.

[carlosgalvezp]

I think this argument then also applies to #43786 and possibly other cases.

Yes, that sounds like the same use case. Whatever we decide should apply to that as well.

[chrchr-github]

Another observation: readability-redundant-string-init is basically a special case for string classes, which flags member and non-member variables. I'm not sure why readability-redundant-member-init is constrained to only member variables.