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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone 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
----------------
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.)


================
Comment at: clang/test/SemaCXX/using-if-exists.cpp:60
+  typedef int mt;
+};
+
----------------
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?


================
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}}
+
----------------
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?


================
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}}
+};
+
----------------
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? 


================
Comment at: clang/test/SemaCXX/using-if-exists.cpp:199
+using ::fopen UIE; // expected-note {{using declaration annotated with 'using_if_exists' here}}
+using ::size_t UIE; // expected-note {{using declaration annotated with 'using_if_exists' here}}
+}
----------------
IMHO this test would be clearer if it used obviously fake names, e.g. `using ::fake_FILE`, `using ::fake_printf`, etc.


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

https://reviews.llvm.org/D90188