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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
