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

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

A few questions. I'm not familiar enough with the code to accept the patch.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:5266
+  let Content = [{
+The using_if_exists attribute applies to a using-declaration. It allows
+programmers to import a declaration that potentially does not exist, instead
----------------
`using_if_exists ` please add two backticks before and after the attribute name so it renders nicer.





================
Comment at: clang/include/clang/Basic/AttrDocs.td:5273
+  namespace empty_namespace {};
+  using empty_namespace::does_not_exist __attribute__((using_if_exists)); // no error!
+
----------------
Can you add an example using `[[clang::using_if_exists]]` or use that instead of this attribute?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5273
+  namespace empty_namespace {};
+  using empty_namespace::does_not_exist __attribute__((using_if_exists)); // no error!
+
----------------
Mordante wrote:
> Can you add an example using `[[clang::using_if_exists]]` or use that instead of this attribute?
Why is the attribute placed here? I would expect the attribute before the declaration `[[clang::using_if_exists]] using empty_namespace::does_not_exist;`


================
Comment at: clang/lib/Sema/SemaDecl.cpp:500
+    // Recover with 'int'
+    T = Context.IntTy;
   } else if (AllowDeducedTemplate) {
----------------
Why do you want recover instead of returning a `nullptr`?


================
Comment at: clang/test/SemaCXX/using-if-exists-attr.cpp:1
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only %s -verify
+
----------------
Does the test require C++20? If so can you use `-std=c++20` since Clang supports it, same for the other test.


================
Comment at: clang/test/SemaCXX/using-if-exists-attr.cpp:9
+
+// FIXME: Should we support the C++ spelling here?
+using NS::x [[clang::using_if_exists]]; // expected-error {{an attribute list cannot appear here}}
----------------
I would prefer to allow the C++ spelling, the square brackets are also supported in C when using `-fdouble-square-bracket-attributes`. (Not too useful in this case since C doesn't support namespaces.) But as mentioned before I prefer the attribute before the `using ` declaration.


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

https://reviews.llvm.org/D90188