llvm / llvm-project

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

Clang++ issues warning that SFINAE-disabled conversion function will never be used. #59470

Open hannesweisbach opened 1 year ago

hannesweisbach commented 1 year ago

Hello everyone,

I have a short sample program:

#include <type_traits>

template <typename T>
struct d {
    template <bool Dependent = false, class = std::enable_if_t<(Dependent || false)>>
    operator d<T>() { return *this; }
};

int main() { [[maybe_unused]] d<int> a; }

Compiling gives the following warning:

$ clang++-12  ./example.cc
./example.cc:6:5: warning: conversion function converting 'd<int>' to itself will never be used [-Wclass-conversion]
    operator d<T>() { return *this; }
    ^
./example.cc:9:38: note: in instantiation of template class 'd<int>' requested here
int main() { [[maybe_unused]] d<int> a; }
                                     ^
1 warning generated.

I used Clang++ 12 from Ubuntu for the above case. Here's also a link with the source in Compiler Explorer and the compiler set to Clang++ 15: https://godbolt.org/z/9WaGenEP5

My expectation would be - given that the conversion operator is disabled by SFINAE - that no warning should be issued. It seems to me that the warning is generated from all member functions, but no actual instantiation/overload set is formed?

Am I wrong to assume there should not be a diagnostic issued?

This is of course a MWE. In my code struct d is a more complicated class template with more template parameters. I realize it's not your job to teach me C++, but assuming that Clang shows the correct behaviour here, is there a standard conformant way of offering conversion operators for certain class template instantiations that would also compile warning-free under -Wclass-conversion with Clang?

Thanks and best regards, Hannes

llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-frontend

dwblaikie commented 1 year ago

In this case the return type is d<T> which is the same as the outer instantiation - so presumably there's /no/ case where this conversion operator would be usable, right? (like, what value of T do you want this code to do anything)

If there were conversions you wanted to some d<U> where T != U then you could have code like this: https://godbolt.org/z/v5GdserKT ?

hannesweisbach commented 1 year ago

Thank you for your reply, David.

I might have shorted the example a bit too much and obfuscated my use-case. Yes, in the example given, the conversion operator would be of no use. Your example is very close. I want to do something like this: https://godbolt.org/z/M3d5KfPWj.

I think, for now I'll go with your suggestion and restrict a bit more: https://godbolt.org/z/qjqhWbdfd.

Still, it feels "weird" to me to get a warning for a conversion operator that can't be invoked.

PS: I just tried restricting U to std::true_type: https://godbolt.org/z/ncqdMezx4 and that works as well. I'm not sure, but I think in the end-result this is equivalent to operator d<std::false_type>() or not?

dwblaikie commented 1 year ago

Still, it feels "weird" to me to get a warning for a conversion operator that can't be invoked.

Yeah, agreed with your examples - seems plausible the warning shouldn't be emitted when SFINAE would disable it anyway.

PS: I just tried restricting U to std::true_type: https://godbolt.org/z/ncqdMezx4 and that works as well. I'm not sure, but I think in the end-result this is equivalent to operator d() or not?

Yeah, seems OK to me. The previous version ( https://godbolt.org/z/qjqhWbdfd ) is probably fine too.

hannesweisbach commented 1 year ago

The solution is good enough for me, thanks for the help!

Up to you or the clang team, if this issue remains open. If there is interest, I might have some time over the holidays to improve the circumstances under which the warning is printed.

shafik commented 1 year ago

This looks like: https://github.com/llvm/llvm-project/issues/43954

shafik commented 1 year ago

I don't think we would want to disable this diagnostic just because the member function is SFINAE disabled.

Also note gcc also diagnoses for this code: https://godbolt.org/z/dvqKhEh4z

CC @AaronBallman

dwblaikie commented 1 year ago

I don't think we would want to disable this diagnostic just because the member function is SFINAE disabled.

Why's that? This seems like fairly reasonable code - where you wan to provide a conversion to other template instantiations of the same template, and it's pretty harmless that the self-conversion will not be used. I guess a different way to phrase the diagnostic suppression might be: Don't warn on potentially self-referential conversion templates, it's probably not a bug/surprising to the developer that they won't be used for self conversion, but still useful for conversion to other instantiations.

Also note gcc also diagnoses for this code: https://godbolt.org/z/dvqKhEh4z

Fair - though we do diverge from GCC regularly where we think we can do better/lower the false positive rate, etc.

CC @AaronBallman

hannesweisbach commented 1 year ago

This looks like: #43954

Yes it does. Thanks!

I don't think we would want to disable this diagnostic just because the member function is SFINAE disabled.

Why not? It breaks -Werror-builds (needlessly?)

Also note gcc also diagnoses for this code: https://godbolt.org/z/dvqKhEh4z

But not this: https://godbolt.org/z/q1TGKK5K8 - clang diagnoses both. Ok, clang is more consistent, I'd say, which is not necessarily a bad thing.

Also, I think the warning only occurs if the return type has not at least one template parameter that has to be deduced (dependent type?). The follwing also gives not a warning in clang: https://godbolt.org/z/4Mxx1G83n

royjacobson commented 1 year ago

I've actually hit a false positive of this in an internal span implementation, it had something like this:

template <typename T>
struct d {
    template <bool Dependent = false, class = std::enable_if_t<(Dependent || false)>>
    operator d<const T>() const { return *this; }
};
int main() {
    d<const int> d1;
}

where const T is sometimes equal to T but not always.