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