[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

Sat Apr 13 06:54:15 PDT 2019

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 at -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