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