rsmith marked 2 inline comments as done.
rsmith added inline comments.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:4321
+ isCXXDeclarationSpecifier(ITC_Never, TPResult::True) !=
+ TPResult::True) ||
+ (!getLangOpts().CPlusPlus && !isDeclarationSpecifier(ITC_Yes))) {
----------------
Rakete1111 wrote:
> rsmith wrote:
> > 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.
> Do you know why this isn't allowed in `operator` ids?
There's already been discussion of that on the core reflector, and people seem
to agree it's an oversight in the wording. If you want to accept that here, I
think that's fine, under the assumption that this will be fixed by DR. (If you
want to follow the wording-as-moved, that's fine too.)
================
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) ||
----------------
Rakete1111 wrote:
> rsmith wrote:
> > 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.
> Yeah it does the break the second. Would just checking whether a `friend` is
> there be good enough? clang doesn't seem to actively propose to add a friend
> if there's one missing, so if we add a `IsFriend` parameter and then check if
> it is set than always return `false`, it should work. Is that acceptable? It
> would break the nice error message you get if you write `friend C(int);`, but
> if we restrict it when `isDeclarationSpecifier` return false (with `Never`)
> would that be better (that's what I'm doing right now)?
>
> Another solution would be to tentatively parse the whole thing, but that can
> be pretty expensive I believe.
This seems a little fragile against future grammar changes, but I think the
`IsFriend` check is correct -- I *think* the only way we can see a
//qualified-id// here in a valid non-constructor, non-nested-name-specifier
case is in a friend declaration.
================
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).
----------------
Rakete1111 wrote:
> Rakete1111 wrote:
> > rsmith wrote:
> > > This FIXME is concerning. Is this a problem with this patch? (Is the
> > > FIXME wrong now?)
> > Yes you're right, I believe it not longer applies due to the change in
> > ParseDecl.cpp in ParseDeclarationSpecifiers.
> Although I'm not that sure I fully understand what that comments alludes to.
The example would be something like this:
```
template<typename T> struct X {
template<typename U> struct Y { Y(); using V = int; };
template<typename U> typename Y<U>::V f();
};
template<typename T> template<typename U>
X<T>::Y<U>::V X<T>::f() {}
```
Clang trunk today points out that the `typename` keyword is missing on the
final line, but fails to point out that the `template` keyword is also missing.
The reason is that in the case where that construct is valid:
```
template<typename T> template<typename U>
X<T>::Y<U>::Y() {}
```
... we are "entering the context" of the //nested-name-specifier//, which means
we don't need a `template` keyword.
If the FIXME is fixed, then we should diagnose the missing `template` in the
above program.
Also, because we don't rebuild the //nested-name-specifier// as a dependent
nested name specifier in this case, we fail to match it against the declaration
in the class, so in my example above, we also produce an "out-of-line
definition does not match" error.
A closely-related issue can be seen in an example such as:
```
template<typename T> struct X {
template<typename U> struct Y { Y(); typedef void V; };
template<typename U> typename Y<U>::V::W f();
};
template<typename T> template<typename U>
X<T>::template Y<U>::V::W X<T>::f() { return 0; }
```
Here, we produce a completely bogus error:
```
<stdin>:6:13: error: 'X::Y::V' (aka 'void') is not a class, namespace, or
enumeration
X<T>::template Y<U>::V::W X<T>::f() { return 0; }
^
```
... because we parse this in "entering context" mode and so resolve
`X<T>::Y<U>::V` to the type in the primary template (that is, `void`). That's
wrong: we should defer resolving this name to a type until we know what `T` and
`U` are, or until we know that we're *actually* entering the context.
Specifically, the above program can be extended as follows:
```
template<> template<> struct X<int>::Y<int> {
struct V { using W = int; };
};
void call(X<int> x) { x.f<int>(); } // OK, f returns int
```
The above program should be accepted by this patch, if the FIXME is indeed now
fixed. Please add it as a testcase :)
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
