[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/Sema.h:1784
                          bool IsClassTemplateDeductionContext = true,
+                         bool AllowImplicitTypename = false,
                          IdentifierInfo **CorrectedII = nullptr);
----------------
It's dangerous to add a `bool` parameter like this, in a long list of `bool` parameters with default arguments. Please add an `enum` for `AllowImplicitTypename` and use it instead of the boolean flag.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2652-2654
+      // But only if we are not in a function prototype scope.
+      if (getCurScope()->isFunctionPrototypeScope())
+        break;
----------------
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.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:4859
     // recurse to handle whatever we get.
-    if (TryAnnotateTypeOrScopeToken())
+    if (TryAnnotateTypeOrScopeToken(!getCurScope()->isTemplateParamScope()))
       return true;
----------------
This seems surprising to me; I'd expect to have an implicit `typename` here approximately when not `DisambiguatingWithExpression`. Also basing this off the scope seems wrong, as we can switch into and out of implicit `typename` contexts multiple times within a scope. Eg, in `template<typename T, T::template U<T::V>>` we get an implicit `typename` for `T::template U` but not for `T::V` despite them being in the same scope.

Should the callers of this function be passing in an "implicit `typename`" flag?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5882
       if (getLangOpts().CPlusPlus && D.mayBeFollowedByCXXDirectInit()) {
+        bool AllowImplicitTypename = false;
+        if (D.getCXXScopeSpec().isSet())
----------------
A citation of the relevant wording here would be useful.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:6444
+
+  bool AllowImplicitTypename;
+  if (D.getContext() == DeclaratorContext::MemberContext ||
----------------
Likewise a citation of the relevant rule here would be useful.


================
Comment at: clang/lib/Parse/ParseTentative.cpp:1280
+                                  bool AllowImplicitTypename) {
+  const bool NoImplicitTypename = !HasMissingTypename || !AllowImplicitTypename;
+
----------------
This seems unclear to me.

I think we should be passing `AllowImplicitTypename` directly to `TryAnnotate...`, rather than taking `HasMissingTypename` into account -- `AllowImplicitTypename` reflects the language rules, whereas `HasMissingTypename` is just an error recovery tool, so `HasMissingTypename` should not have any influence on how we annotate the token stream unless we actually detect an error.


================
Comment at: clang/lib/Parse/ParseTentative.cpp:1342
     // We annotated this token as something. Recurse to handle whatever we got.
     return isCXXDeclarationSpecifier(BracedCastResult, HasMissingTypename);
   }
----------------
These recursive steps need to pass in the `AllowImplicitTypename` flag. Maybe the default argument should be removed for safety.


================
Comment at: clang/lib/Parse/Parser.cpp:1490
 Parser::AnnotatedNameKind
 Parser::TryAnnotateName(bool IsAddressOfOperand,
                         std::unique_ptr<CorrectionCandidateCallback> CCC) {
----------------
Have you checked that this is never called in an implicit typename context?

Please update the documentation for this to say that it can never be called in such a context, or add an `AllowImplicitTypename` parameter as necessary.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:9503
+  QualType T = CheckTypenameType(
+      TypenameLoc.isValid() || IsImplicitTypename ? ETK_Typename : ETK_None,
+      TypenameLoc, QualifierLoc, II, IdLoc);
----------------
Please parenthesize the left-hand-side of this ternary operator.


================
Comment at: clang/test/CXX/drs/dr1xx.cpp:61
     struct B { typedef int X; };
-    B::X x; // expected-error {{missing 'typename'}}
+    B::X x; // expected-error {{implicit 'typename' is a C++2a extension}}
     struct C : B { X x; }; // expected-error {{unknown type name}}
----------------
For each of these `test/CXX/drs` tests that you change, please add a C++20 `RUN:` line and update the diagnostic expectations to match in pre- and post-C++20 mode as applicable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53847