riccibruno marked 11 inline comments as done. riccibruno added a comment. In D60570#1465478 <https://reviews.llvm.org/D60570#1465478>, @Quuxplusone wrote:
> As you're making tests for ADL corner cases, you might also consider testing > the interactions between ADL and defaulted function parameters, e.g. > https://godbolt.org/z/vHnyFl > It looks like everyone (except MSVC) already gets that stuff right (or at > least portable-between-the-big-three). I bet the behavior naturally falls out > of some other rules; you might say "there's no way that could possibly break, > so we don't need to test it," and I'd accept that. I think that as you say this falls out of the other rules. ================ Comment at: test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:77 + + // associated class: itself, lambda + namespace X5 { ---------------- Quuxplusone wrote: > 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.) I am not sure, but I see no harm in adding it here. ================ Comment at: test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:144 + + // template template argument + namespace X3 { ---------------- Quuxplusone wrote: > 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'}} > } > ``` Isn't that already covered by the test in `adl_class_template_specialization_type::X1` ? I agree that a test for class type NTTP will be needed in C++2a, but for now this feature is not yet implemented. ================ 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}} + } ---------------- Quuxplusone wrote: > 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. :) I don't understand your objection to the template-id rule of CWG 997 (which is what `f(g2<N::S>)` tests). It seems to me that the following should be well-formed (https://godbolt.org/z/J5uhQ-) : ``` namespace N { template <typename T> void f(T &&); struct S {}; } template <typename T> struct C {}; template <typename T> void g(); void test() { f(C<N::S>{}); // ok f(g<N::S>); // should be ok (and is ok according to the spec) } ``` ================ 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'}} + } ---------------- Quuxplusone wrote: > ...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. Hmm... I am not sure what gcc is thinking here. The order should not matter according to the beginning of [basic.lookup.argdep]p3. ================ 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 ---------------- Quuxplusone wrote: > Nit: 80-column lines here and above would be nice. :) I am usually a little bit more flexible on the 80-columns rule in tests, but you are right. Fixed. 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
