Quuxplusone added inline comments.
================
Comment at:
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:77
+
+ // associated class: itself, lambda
+ namespace X5 {
----------------
Do you also have a test somewhere to verify that the parameter and return types
of a lambda's `operator()` do not contribute to the associated types of the
lambda type itself? That is,
```
// https://godbolt.org/z/g_oMOA
namespace N {
struct A {};
template<class T> constexpr int f(T) { return 1; }
}
constexpr int f(N::A (*)()) { return 2; }
constexpr int f(void (*)(N::A)) { return 3; }
void test() {
constexpr auto lambda = []() -> N::A { return {}; };
static_assert(f(lambda) == 2);
constexpr auto lambda2 = [](N::A) {};
static_assert(f(lambda2) == 3);
}
```
Clang does handle this correctly; I'm just asking for it to be tested, if it's
not already. (I might have overlooked an existing test.)
================
Comment at:
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:144
+
+ // template template argument
+ namespace X3 {
----------------
I think for completeness there should be a "negative test" for non-type
template arguments:
```
namespace X4 {
template <auto NT> struct C {};
namespace N {
struct Z {
enum E { E0 };
void X4_f(C<E::E0>);
};
enum E { E0 };
void X4_g(C<E::E0>);
}
}
void test4() {
X4::C<X4::N::E::E0> c1;
X4::C<X4::N::Z::E::E0> c2;
X4_f(c1); // expected-error{{undeclared identifier 'X4_f'}}
X4_g(c2); // expected-error{{undeclared identifier 'X4_g'}}
}
```
In C++2a, user-defined NTTPs will become possible, so we'll want another test
for something like
```
// https://godbolt.org/z/MfWG8C
namespace X4 {
template<auto NT> struct C {};
namespace N {
struct Z {
int i;
constexpr Z(int i): i(i) {}
auto operator<=>(const Z&) const = default;
};
void X4_f(C<Z(0)>);
}
}
void test4() {
X4::C<X4::N::Z(0)> c1;
X4_f(c1); // expected-error{{undeclared identifier 'X4_f'}}
}
```
================
Comment at:
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:304
+ static_assert(f(g3) == 4, ""); // FIXME: Also well-formed from the
union rule.
+ // expected-error@-1 {{use of
undeclared}}
+ }
----------------
riccibruno wrote:
> Quuxplusone wrote:
> > I see how `g3` matches the example in CWG997
> > http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#997
> > However, I don't see how CWG997's resolution actually affected this example
> > in the slightest. The wording inserted for CWG997 was, "Additionally, if
> > the aforementioned set of overloaded functions is named with a template-id,
> > its associated classes and namespaces are those of its type
> > template-arguments and its template template-arguments." That makes e.g.
> >
> > f(g3<N::S>)
> >
> > consider `N::f`, because `N::S` is a "type template-argument" of the
> > template-id `g3<N::S>` which names the set of overloaded functions. But it
> > doesn't do anything at all to `f(g3)` because `g3` is not a template-id and
> > doesn't have any template-arguments.
> >
> > This piece of ADL is implemented only by GCC (not EDG, Clang, or MSVC), and
> > personally I would very much like to keep it that way. We know there's no
> > real-world code that expects or relies on CWG997 — because such code would
> > never work in practice except on GCC. Let's keep it that way! As soon as
> > you implement a crazy arcane rule, such that code _could_ portably rely on
> > it, code _will start_ relying on it... and then we'll never be able to
> > simplify the language.
> > Case in point: the piece of ADL described in this blog post --
> > https://quuxplusone.github.io/blog/2019/04/09/adl-insanity-round-2/
> > As soon as the above-described arcane ADL rule was implemented in GCC and
> > Clang, Boost.Hana started relying on it; and now the rule is "locked in" to
> > the paper standard because there's real-world code relying on it.
> > Personally I'd like to _keep_ real-world code from relying on CWG997, until
> > someone figures out what CWG was thinking when they added it.
> I think that the relevant part of CWG 997 is the removal of the restriction
> on non-dependent parameter types. Sure, `g3` is not a `template-id`, but it
> refers to an overload set which contains the second `g3`, and one of the
> parameter of this second `g3` is `N::Q<T>`.
>
> I don't think this is a surprising rule. It matches the general intuition
> that for function types ADL is done based on the function parameter types and
> return type. Not having this rule introduces a difference between function
> templates and functions in overload sets. Consider
> https://godbolt.org/z/UXHqm2 :
> ```
> namespace N {
> struct S1 {};
> template <typename> struct S2 {};
>
> void f(void (*g)());
> }
>
> void g1(); // #1
> void g1(N::S1); // #2
>
> void g2(); // #3
> template <typename T> void g2(N::S2<T>); // #4
>
> void test() {
> f(g1); // ok, g1 is #1
> f(g2); // should be ok, g2 is #3
> }
> ```
> I think that the relevant part of CWG 997 is the removal of the restriction
> on non-dependent parameter types.
Ah, I had missed the removal of the word `(non-dependent)` in my reading of
CWG997. So just that one-word removal is what fixed their example, and is what
you're testing with `g3`.
I still object to `g2` — I would like that `FIXME` to say `PLEASEDONTFIXME` or
something. :)
================
Comment at: test/CXX/basic/basic.lookup/basic.lookup.argdep/p3.cpp:50
+ extern void f(char); // expected-note {{candidate}}
+ f(s); // expected-error {{no matching function for call to 'f'}}
+ }
----------------
...But if you put the `using M::f;` _after_ the `extern void f(char);`, then
GCC believes it's okay. https://godbolt.org/z/DghSTM
You should definitely have a test for the using-after-extern case, just to make
sure it doesn't ICE or anything.
================
Comment at: test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp:96
+// they are not visible during an ordinary lookup
+// (Note: For the friend declaration to be visible, the corresponding class
must be
+// included in the set of associated classes. Merely including the namespace
in
----------------
Nit: 80-column lines here and above would be nice. :)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60570/new/
https://reviews.llvm.org/D60570
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
