[PATCH] D53847: [C++2a] P0634r3: Down with typename!

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Sema/DeclSpec.h:1753-1758
+// Describes whether the current context is a context where an implicit
+// typename is allowed (C++2a [temp.res]p5]).
+enum ImplicitTypenameContext {
+  ITC_Never,
+  ITC_Yes,
+};
----------------
Consider using an `enum class` here.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2652-2654
+      // But only if we are not in a function prototype scope.
+      if (getCurScope()->isFunctionPrototypeScope())
+        break;
----------------
rsmith wrote:
> Can you split out this error recovery improvement and commit it separately before the rest of this work? It doesn't appear to have any dependency on the rest of the change.
Looks like you need to rebase; the change was committed but is still in the latest version of this patch.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:4321
+             isCXXDeclarationSpecifier(ITC_Never, TPResult::True) !=
+                 TPResult::True) ||
+            (!getLangOpts().CPlusPlus && !isDeclarationSpecifier(ITC_Yes))) {
----------------
It seems like a wording oversight that we don't assume `typename` in an //enum-base//. Probably would be good to raise this on the core reflector.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:4322
+                 TPResult::True) ||
+            (!getLangOpts().CPlusPlus && !isDeclarationSpecifier(ITC_Yes))) {
           // We'll parse this as a bitfield later.
----------------
Using a different `ITC` value for non-C++ compilations seems surprising. (It should never make any difference outside of C++, but leaves the reader wondering why the two are different.) Can we use `ITC_Never` here for consistency?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5100-5101
   bool IsConstructor = false;
-  if (isDeclarationSpecifier())
+  if (isDeclarationSpecifier(ITC_Never))
     IsConstructor = true;
   else if (Tok.is(tok::identifier) ||
----------------
Oh, this could be a problem.

If this *is* a constructor declaration, then this is implicit typename context: this is either a "//parameter-declaration// in a //member-declaration//" ([temp.res]/5.2.3) or a "//parameter-declaration// in a //declarator// of a function or function template declaration whose //declarator-id// is qualified". But if it's *not* a constructor declaration, then this is either the //declarator-id// of a declaration or the //nested-name-specifier// of a pointer-to-member declarator:

```
template<typename T>
struct C {
  C(T::type); // implicit typename context
  friend C (T::fn)(); // not implicit typename context, declarator-id of friend declaration
  C(T::type::*x)[3]; // not implicit typename context, pointer-to-member type
};
```

I think we need to use `ITC_Yes` here, in order to correctly disambiguate the first example above. Please add tests for the other two cases to make sure this doesn't break them -- but I'm worried this **will** break the second case, because it will incorrectly annotate `T::fn` as a type.


================
Comment at: clang/lib/Sema/Sema.cpp:2219
+  
+  D.setPrevLookupResult(llvm::make_unique<LookupResult>(std::move(LR)));
+  return Result;
----------------
Consider moving the `make_unique` earlier (directly before `LookupQualifiedName`) to avoid needing to move the `LookupResult` object into different storage here.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:3369
     if (!LookupCtx && isDependentScopeSpecifier(SS)) {
-      Diag(SS.getBeginLoc(), diag::err_typename_missing_template)
-        << SS.getScopeRep() << TemplateII->getName();
-      // Recover as if 'typename' were specified.
+      // C++2a relaxes some of those restrictinos in [temp.res]p5.
+      if (getLangOpts().CPlusPlus2a)
----------------
Are there any cases where we would call this for which C++20 still requires a `typename` keyword? Should this function be passed an `ImplicitTypenameContext`?


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:3377-3379
       // FIXME: This is not quite correct recovery as we don't transform SS
       // into the corresponding dependent form (and we don't diagnose missing
       // 'template' keywords within SS as a result).
----------------
This FIXME is concerning. Is this a problem with this patch? (Is the FIXME wrong now?)


================
Comment at: clang/test/CXX/drs/dr5xx.cpp:485
 namespace dr542 { // dr542: yes
-#if __cplusplus >= 201103L
+#if __cplusplus >= 201103L && __cplusplus <= 201703L
   struct A { A() = delete; int n; };
----------------
A comment here explaining that `A` and `B` stop being aggregates in C++20 would be nice. (Nicer would be changing the testcase so it still tests the relevant rule in C++20 mode, if that's possible...)


================
Comment at: clang/test/CXX/temp/temp.res/p5.cpp:1
+// RUN: %clang_cc1 -std=c++2a -pedantic -verify %s
+
----------------
Please add tests for enum-base and conversion-type-id:

```
template<typename T> struct A {
  enum E : T::type {};
  operator T::type() {}
  void f() { this->operator T::type(); }
};
```

... which should currently be rejected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53847