george.burgess.iv created this revision.
george.burgess.iv added a reviewer: rsmith.
george.burgess.iv added a subscriber: cfe-commits.
This patch is meant to be applied instead of D18425, but is a new review
because we're taking an entirely different approach.
This patch makes us fail `enable_if` conditions in a value dependent context,
if not all values are present when we're trying to evaluate the `enable_if`.
e.g.
```
void foo(int i) __attribute__((enable_if(i, "")));
template <int N> void callFoo() { foo(N); } // Error: No viable overload
(emitted before any instantiations of callFoo are made), because the call to
foo is resolved prior to instantiating callFoo, so we don't have a single value
for N.
```
Our current behavior is that we'll happily resolve `foo` to the only existing
`foo` overload, and be totally fine with `callFoo<0>();` as a result, which is
incorrect. Were there two `foo` overloads with different `enable_if`
attributes, we would always emit an error about an ambiguous overload set.
A complete, consistent fix for this would require that we're able to
arbitrarily defer overload resolution. Additionally, we would need to handle
type dependence appearing out of nowhere, as in:
```
double bar(int N) __attribute__((enable_if(N, "")));
void *bar(int N) __attribute__((enable_if(!N, "")));
template <int N>
auto callBar() { return bar(N); }
```
Which seems like a very large, bug-prone change.
Given that it seems no one has tried to use `enable_if` in a value-dependent
context yet (if they were, I'd assume we would have received a bug report about
`bar(N)` always being ambiguous by now), I think this is the least bad of all
of our options.
http://reviews.llvm.org/D20130
Files:
include/clang/Basic/AttrDocs.td
lib/Sema/SemaOverload.cpp
test/SemaCXX/enable_if.cpp
Index: test/SemaCXX/enable_if.cpp
===================================================================
--- test/SemaCXX/enable_if.cpp
+++ test/SemaCXX/enable_if.cpp
@@ -116,9 +116,9 @@
void g() { f(); }
};
-int fn3(bool b) __attribute__((enable_if(b, "")));
+int fn3(bool b) __attribute__((enable_if(b, ""))); // FIXME: This test should net 0 error messages.
template <class T> void test3() {
- fn3(sizeof(T) == 1);
+ fn3(sizeof(T) == 1); // expected-error{{no matching function for call to 'fn3'}} expected-note@-2{{candidate disabled}}
}
template <typename T>
@@ -138,7 +138,7 @@
void h(int);
template <typename T> void outer() {
void local_function() __attribute__((enable_if(::h(T()), "")));
- local_function();
+ local_function(); // expected-error{{no matching function for call to 'local_function'}} expected-note@-1{{candidate disabled}}
};
namespace PR20988 {
@@ -158,9 +158,9 @@
fn2(expr); // expected-error{{no matching function for call to 'fn2'}}
}
- int fn3(bool b) __attribute__((enable_if(b, "")));
+ int fn3(bool b) __attribute__((enable_if(b, ""))); // FIXME: This test should net 0 error messages.
template <class T> void test3() {
- fn3(sizeof(T) == 1);
+ fn3(sizeof(T) == 1); // expected-error{{no matching function for call to 'fn3'}} expected-note@-2{{candidate disabled}}
}
}
@@ -386,3 +386,34 @@
f.bar(1, 2); // expected-error{{too many arguments}}
}
}
+
+// Ideally, we should be able to handle value-dependent expressions sanely.
+// Sadly, that isn't the case at the moment.
+namespace dependent {
+int error(int N) __attribute__((enable_if(N, ""))); // expected-note{{candidate disabled}}
+int error(int N) __attribute__((enable_if(!N, ""))); // expected-note{{candidate disabled}}
+template <int N> int callUnavailable() {
+ return error(N); // expected-error{{no matching function for call to 'error'}}
+}
+
+constexpr int noError(int N) __attribute__((enable_if(N, ""))) { return -1; }
+constexpr int noError(int N) __attribute__((enable_if(!N, ""))) { return -1; }
+constexpr int noError(int N) { return 0; }
+
+template <int N>
+constexpr int callNoError() { return noError(N); }
+static_assert(callNoError<0>() == 0, "");
+static_assert(callNoError<1>() == 0, "");
+
+template <int N> constexpr int templated() __attribute__((enable_if(N, ""))) {
+ return 1;
+}
+
+constexpr int A = templated<0>(); // expected-error{{no matching function for call to 'templated'}} expected-note@-4{{candidate disabled}}
+static_assert(templated<1>() == 1, "");
+
+template <int N> constexpr int callTemplated() { return templated<N>(); }
+
+constexpr int B = callTemplated<0>(); // expected-error{{initialized by a constant expression}} expected-error@-2{{no matching function for call to 'templated'}} expected-note{{in instantiation of function template}} expected-note@-9{{candidate disabled}}
+static_assert(callTemplated<1>() == 1, "");
+}
Index: lib/Sema/SemaOverload.cpp
===================================================================
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -5984,7 +5984,6 @@
SFINAETrap Trap(*this);
SmallVector<Expr *, 16> ConvertedArgs;
bool InitializationFailed = false;
- bool ContainsValueDependentExpr = false;
// Convert the arguments.
for (unsigned I = 0, E = Args.size(); I != E; ++I) {
@@ -6006,7 +6005,6 @@
break;
}
- ContainsValueDependentExpr |= R.get()->isValueDependent();
ConvertedArgs.push_back(R.get());
}
@@ -6027,7 +6025,6 @@
InitializationFailed = true;
break;
}
- ContainsValueDependentExpr |= R.get()->isValueDependent();
ConvertedArgs.push_back(R.get());
}
@@ -6037,18 +6034,14 @@
for (auto *EIA : EnableIfAttrs) {
APValue Result;
- if (EIA->getCond()->isValueDependent()) {
- // Don't even try now, we'll examine it after instantiation.
- continue;
- }
-
+ // FIXME: This doesn't consider value-dependent cases, because doing so is
+ // very difficult. Ideally, we should handle them more gracefully.
if (!EIA->getCond()->EvaluateWithSubstitution(
- Result, Context, Function, llvm::makeArrayRef(ConvertedArgs))) {
- if (!ContainsValueDependentExpr)
- return EIA;
- } else if (!Result.isInt() || !Result.getInt().getBoolValue()) {
+ Result, Context, Function, llvm::makeArrayRef(ConvertedArgs)))
+ return EIA;
+
+ if (!Result.isInt() || !Result.getInt().getBoolValue())
return EIA;
- }
}
return nullptr;
}
Index: include/clang/Basic/AttrDocs.td
===================================================================
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -283,6 +283,32 @@
ptr = &i; // OK: 'TrueConstant' is a truthy constant
ptr = &j; // error: 'FalseConstant' is a constant, but not truthy
}
+
+Because ``enable_if`` evaluation happens during overload resolution,
+``enable_if`` may give unintuitive results when used with templates, depending
+on when overloads are resolved. In the example below, clang will emit a
+diagnostic about no viable overloads for ``foo`` in ``bar``, but not in ``baz``:
+
+.. code-block:: c++
+
+ double foo(int i) __attribute__((enable_if(i > 0, "")));
+ void *foo(int i) __attribute__((enable_if(i <= 0, "")));
+ template <int I>
+ auto bar() { return foo(I); }
+
+ template <typename T>
+ auto baz() { return foo(T::number); }
+
+ struct WithNumber { constexpr static int number = 1; };
+ void callThem() {
+ bar<sizeof(WithNumber)>();
+ baz<WithNumber>();
+ }
+
+This is because, in ``bar``, ``foo`` is resolved prior to template
+instantiation, so the value for ``I`` isn't known (thus, both ``enable_if``
+conditions for ``foo`` fail). However, in ``baz``, ``foo`` is resolved during
+template instantiation, so the value for ``T::number`` is known.
}];
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
