llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.1k stars 12.01k forks source link

[clang-tidy] `bugprone-forwarding-reference-overload` doesn't know about concepts #58230

Closed h-2 closed 1 year ago

h-2 commented 2 years ago
struct small_vector
{
    constexpr small_vector() noexcept                                 = default; 
    constexpr small_vector(small_vector const &) noexcept             = default;
    constexpr small_vector(small_vector &&) noexcept                  = default;
    constexpr small_vector & operator=(small_vector const &) noexcept = default;
    constexpr small_vector & operator=(small_vector &&) noexcept      = default;
    ~small_vector() noexcept                                          = default;

    template <meta::different_from<small_vector> other_range_t>
        requires(std::ranges::input_range<other_range_t>)
    explicit constexpr small_vector(other_range_t && range) 
    {}

    // ...

};

results in

small_vector.hpp:186:24: warning: constructor accepting a forwarding reference can hide the copy and move constructors [bugprone-forwarding-reference-overload]
    explicit constexpr small_vector(other_range_t && range) noexcept(is_noexcept) :
                       ^
/home/hannes/devel/biocpp-core/test/clang_tidy/../../include/bio/ranges/container/small_vector.hpp:73:15: note: copy constructor declared here
    constexpr small_vector(small_vector const &) noexcept             = default; //!< Defaulted.
              ^
/home/hannes/devel/biocpp-core/test/clang_tidy/../../include/bio/ranges/container/small_vector.hpp:74:15: note: move constructor declared here
    constexpr small_vector(small_vector &&) noexcept                  = default; //!< Defaulted.

although the constraints prevent the constructor from ever being instantiated with either small_vector const & or small_vector.

edit:

Ubuntu package: 16~++20221005031330+a3a741c0bbb2-1~exp1~20221005151435.464

llvmbot commented 2 years ago

@llvm/issue-subscribers-clang-tidy

Izaron commented 2 years ago

Thanks for the issue! A possible fix - https://reviews.llvm.org/D135476

h-2 commented 2 years ago

Thanks for coming up with a fix so quickly! Do I understand correctly that it would just ignore all constrained constructors in the future?

That would definitely solve the false-positive warning, although actually evaluating the constraint on the type would be even more helpful, of course. Because, while the warning did return quite a few false-positives in my codebase, it actually did turn up a wrongly constrained constructor which did need fixing.

But I don't know at all how clang-tidy works and whether it is even capable of performing such a check, so I am happy with the current fix, too!

Izaron commented 2 years ago

Do I understand correctly that it would just ignore all constrained constructors in the future?

Hi! Yes, I just ignore all constrained constructors. I had an idea to evaluate constraints too. But the check's code doesn't evaluate constraints for enable_if (and concepts are sort of evolved enable_if), so I decided to not overengineer the check.

Also I don't know if clang-tidy is able to evaluate constraints. I haven't figured it out yet. Let's look what other devs say on this patch =)