[PATCH] D90188: Add support for attribute 'using_if_exists'

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/test/SemaCXX/using-if-exists.cpp:11
+
+using NotNS::x UIE; // expected-error{{use of undeclared identifier 'NotNS'}}
+} // test_basic
----------------
Quuxplusone wrote:
> Do you also have a test for `using NS::NotNS::x UIE;`?
> (Both of these `NotNS` tests produce hard errors; `[[using_if_exists]]` does the silent-ignore thing only when the tippymost identifier is nonexistent, not if some intermediate name is nonexistent. This seems reasonable enough.)
Yeah, this is only about the tippymost name :). New patch adds a test for non-existent names in the middle, which was previously untested.


================
Comment at: clang/test/SemaCXX/using-if-exists.cpp:60
+  typedef int mt;
+};
+
----------------
Quuxplusone wrote:
> An inheritance case not yet covered here errors out with `error: no matching constructor for initialization of 'Derived<Base>'`:
> ```
> struct Base { Base(int); };
> template<class T> struct Derived : T {
>     using T::T [[clang::using_if_exists]];
> };
> Derived<Base> d(1);
> ```
> Does this make sense? Should this attribute perhaps give a hard error if you try to use it to inherit a constructor overload set like this, instead of (apparently) just no-opping the construct?
Hmm, yeah, I don't think that inheriting constructors really make sense with this attribute. New patch adds an error.


================
Comment at: clang/test/SemaCXX/using-if-exists.cpp:79
+  using B::mf UIE; // expected-note {{using declaration annotated with 'using_if_exists' here}}
+  using typename B::mt UIE; // expected-note 2 {{using declaration annotated with 'using_if_exists' here}}
+
----------------
Quuxplusone wrote:
> I notice there's a hard `error: 'using_if_exists' attribute cannot be applied to types` on
> 
>     using B::operator int UIE;
> 
> Any thoughts on how to disambiguate that grammar?
Hmm, that's an interesting case. I'm not sure what the right approach is. I think it would be a bit awkward to decide where to attach the attribute depending on what kind of entities the attribute could potentially apply to. FWIW this isn't a semantic restriction, since you can always just use the prefix syntax: `UIE using B::operator int;`. Maybe @aaron.ballman has some thoughts here?


================
Comment at: clang/test/SemaCXX/using-if-exists.cpp:107
+  using typename Ts::x... UIE; // expected-error 2 {{target of using declaration conflicts with declaration already in scope}} expected-note{{conflicting declaration}} expected-note{{target of using declaration}}
+};
+
----------------
Quuxplusone wrote:
> This is a very good test, but it feels to me as if
> ```
> struct B { static int f(int); };
> struct C { };
> template<class... Ts> struct D : Ts... {
>     using Ts::f... [[clang::using_if_exists]];
> };
> int main() { D<B, C>::f(1); }
> ```
> ought to be //at least// as well-formed as
> ```
> struct B { static int f(int); };
> struct C { static void f(); };
> template<class... Ts> struct D : Ts... {
>     using Ts::f... [[clang::using_if_exists]];
> };
> int main() { D<B, C>::f(1); }
> ```
> I guess what's going on here is that an "unresolved using decl" is considered a separate kind of entity, i.e. `using Ts::f...` will work if //all// `Ts::f...` are variables, or if //all// of them are functions, or if //all// of them are unresolved, but it doesn't work if you have a mix of variables and functions, or variables and unresolveds, or functions and unresolveds. Is that basically correct, and intended? 
> I guess what's going on here is that an "unresolved using decl" is considered a separate kind of entity, i.e. using Ts::f... will work if all Ts::f... are variables, or if all of them are functions, or if all of them are unresolved, but it doesn't work if you have a mix of variables and functions, or variables and unresolveds, or functions and unresolveds. Is that basically correct, and intended?


Yeah, exactly. My mental model for how this attribute works is that an unresolved using-declaration introduces an entity that isn't a function, value, or type, and is incompatible with all of those. Sometimes, that isn't what you want for overloaded functions, like Louis ran into an issue while testing this where `remove(const char *)` in `<cstdio>` being unresolved lead to a compile error on the declaration of `remove(iter, iter, val)` in `<algorithm>`. That seems to basically be the same issue you're seeing in your first test case.

Personally, my inclination is to conservatively just disallow any interplay with an unresolved using-declaration and a function, and if we run into a lot of problems in practice then we can come up with a more nuanced approach for this case without breaking backwards compatibility. WDYT?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90188/new/

https://reviews.llvm.org/D90188