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

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

rsmith added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4462-4463
+  DefaultIgnore;
+def ext_implicit_typename : ExtWarn<"implicit 'typename' is a C++2a extension">,
+  InGroup<CXX2a>;
 
----------------
Please make the start of this diagnostic match the non-extension case:

"missing 'typename' prior to dependent type name %0; implicit 'typename' is a C++2a extension"


================
Comment at: include/clang/Parse/Parser.h:1998
+    DSC_condition, // condition declaration context
+    DSC_block, // declaration within a block
   };
----------------
Maybe `compound_stmt` rather than `block`? The term "block" is unfortunately ambgiuous within Clang due to the Apple Blocks extension using the same name for something else.

... but see below, I don't think we need this at all.


================
Comment at: include/clang/Parse/Parser.h:2054
+    case DeclSpecContext::DSC_template_param:
+    case DeclSpecContext::DSC_template_type_arg:
+    case DeclSpecContext::DSC_normal:
----------------
This doesn't seem right to me. Doesn't this mean that `X<T::U>` will change from being a non-type template argument to being a type template argument?

Maybe that's a bug in the wording paper, though, since that context *is* a /type-id/.


================
Comment at: include/clang/Parse/Parser.h:2055-2056
+    case DeclSpecContext::DSC_template_type_arg:
+    case DeclSpecContext::DSC_normal:
+      return true;
+
----------------
Please don't enable this for the "normal" case. If there are more special cases that need the "implicit typename" treatment, we should explicitly identify them rather than making this the default.

Please also get rid of `DSC_block`, because it's then the same as `DSC_normal` again.)


================
Comment at: lib/Parse/ParseDecl.cpp:2677-2705
 Parser::DeclSpecContext
 Parser::getDeclSpecContextFromDeclaratorContext(DeclaratorContext Context) {
   if (Context == DeclaratorContext::MemberContext)
     return DeclSpecContext::DSC_class;
   if (Context == DeclaratorContext::FileContext)
     return DeclSpecContext::DSC_top_level;
   if (Context == DeclaratorContext::TemplateParamContext)
----------------
Can you convert this into a covering `switch`?


================
Comment at: lib/Parse/ParseDecl.cpp:5842
+
+        if (!IsFunctionDecl && IsAmbiguous) {
+          // We have an ambiguity between a function and a variable:
----------------
This is an interesting approach, and it seems like a useful error-recovery rule, but I'm not really sure I see how it can work as part of an implementation of this paper. We need the "is this a redeclaration of a function with a qualified name?" check when we parse the type of a parameter, not merely for disambiguation. For example, given

```
template<typename T> int f(int, T::x);
```

... we should reject, because `typename` is required in the declaration of the second parameter, even though we can disambiguate this as a function declaration without even looking at those tokens (the `int,` can't be anything else).

It seems to me that we should do this lookup (possibly lazily) at some point before we parse a /qualified-id/ as a parameter type, and feed that into the `getTypeName` call we eventually make for the parameter type. (This should also be given as *input* to the disambiguation we do here, so that we can determine that `template<typename T> int f(T::x)` is a variable but `template<typename T> int N::f(T::x)` is a function.)


================
Comment at: lib/Parse/ParseDecl.cpp:5847-5858
+            LookupResult LR(Actions, D.getIdentifier(), D.getBeginLoc(),
+                            Sema::LookupOrdinaryName,
+                            Sema::ForVisibleRedeclaration);
+            DeclContext *DC = Actions.computeDeclContext(D.getCXXScopeSpec());
+            assert(DC && "couldn't find decl context of qualified name");
+            Actions.LookupQualifiedName(LR, DC);
+
----------------
It's not OK to do this from the parser: you should instead add a function to the `Sema` interface to perform the query you want here ("does this name declare a function / function template?"), and call that from wherever is appropriate in the parser.


================
Comment at: lib/Parse/ParseTentative.cpp:1794-1797
+        // But still mark the declaration as ambiguous, as the tie-breakter is
+        // not perfect.
+        if (IsAmbiguous)
+          *IsAmbiguous = true;
----------------
We should not be setting `InvalidAsDeclaration` to `true` for a missing`typename` in a context where `typename` is implied. That way lies chaos.


================
Comment at: lib/Sema/SemaDecl.cpp:317-320
+        // We need to delay this diagnostic for function parameters, because
+        // at that point we don't know whether the declarator-id of the
+        // function is qualified, which affects the interpretation of
+        // a dependent qualified name.
----------------
I'm very unhappy with this approach. The proposal is careful to make sure that we can always know whether we're in a context where `typename` is optional in advance, and never need to guess. We should propagate that knowledge into all the places that parse types where `typename` is implicit.


================
Comment at: test/CXX/temp/temp.res/p5.cpp:135-141
+namespace NN {
+  inline namespace A { template <typename T> int f(typename T::type); } // expected-note{{previous definition is here}}
+  inline namespace B { template <typename T> int f(T::type); }
+}
+
+template <typename T>
+int NN::f(T::type); // expected-error{{redefinition of 'f' as different kind of symbol}}
----------------
This error doesn't make much sense. We should instead be rejecting due to the lookup for `NN::f` being ambiguous (between a function and a variable).


================
Comment at: test/SemaCXX/MicrosoftCompatibility.cpp:218
 template <class T>
-void function_missing_typename(const T::Type param)// expected-warning {{missing 'typename' prior to dependent type name}}
+void function_missing_typename(const T::Type param)// expected-error {{missing 'typename'}}
 {
----------------
Looks like you broke an MS compatibility feature here. We should continue to accept this under `-fms-compatibility`.


================
Comment at: test/SemaCXX/unknown-type-name.cpp:121-122
 template<typename T>
-A<T>::g() { } // expected-error{{requires a type specifier}}
+A<T>::g() { } // expected-error{{expected unqualified-id}}
+// expected-warning at -1{{implicit 'typename' is a C++2a extension}}
----------------
rsmith wrote:
> This is a diagnostic quality regression. Perhaps that's an inevitable consequence of P0634, but we should at least try to do better.
This is marked "done" but doesn't seem to be done?


Repository:
  rC Clang

https://reviews.llvm.org/D53847