<div dir="ltr">Thank you very much for your review!<div><br></div><div>I updated patch according to your notes. Only passing instance of <span style="font-size:13px">NameSpecRecoveryInfo by pointer remained as it was. </span><span style="font-size:13px">BuildCXXNestedNameSpecifier can be called in context that does not imply error recovery (</span>ClassifyName or IsInvalidUnlessNestedName), null pointer to recovery info indicates that recovery is not needed. Otherwise a separate flag would be needed as a function argument or a member of <span style="font-size:13px">NameSpecRecoveryInfo, it would make the code a bit less clear.</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">Also processing the construct 'enumeration::enumerator' changed if Microsoft extensions are enabled. This construct is an extension maintained in MSVC.</span></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature">Thanks,<br>--Serge<br></div></div>
<br><div class="gmail_quote">2015-01-03 4:30 GMT+06:00 Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Dec 22, 2014 at 3:27 PM, Serge Pavlov <<a href="mailto:sepavloff@gmail.com">sepavloff@gmail.com</a>> wrote:<br>
> Any feedback?<br>
<br>
Sorry for the delayed review -- holiday season can be a bit hectic.<br>
Comments below.<br>
<br>
> Index: include/clang/Basic/DiagnosticParseKinds.td<br>
> ===================================================================<br>
> --- include/clang/Basic/DiagnosticParseKinds.td<br>
> +++ include/clang/Basic/DiagnosticParseKinds.td<br>
> @@ -539,6 +539,10 @@<br>
> "unexpected %0 in function call; perhaps remove the %0?">;<br>
> def err_super_in_using_declaration : Error<<br>
> "'__super' cannot be used with a using declaration">;<br>
> +def err_nested_name_spec_is_enum : Error<<br>
> + "unscoped enumeration name shall not be specified in nested name specifier">;<br>
<br>
We don't typically use "shall" in our diagnostics.<br>
<br>
"unscoped enumeration %0 cannot be specified as a nested name specifier" ?<br>
<br>
> +def err_expected_class_or_namespace_or_enum : Error<"%0 is not a class"<br>
> + "%select{ or namespace|, namespace, or scoped enumeration}1">;<br>
<br>
Neither of these strike me as being parsing diagnostics, but instead<br>
as semantic ones. In both cases, the identifier is correct from a<br>
parsing perspective, but the semantic restrictions are not met. Is<br>
there a reason this is not implemented within<br>
Sema::ActOnCXXNestedNameSpecifier?<br>
<br>
><br>
> // C++ derived classes<br>
> def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;<br>
> Index: include/clang/Sema/Sema.h<br>
> ===================================================================<br>
> --- include/clang/Sema/Sema.h<br>
> +++ include/clang/Sema/Sema.h<br>
> @@ -4610,6 +4610,23 @@<br>
> IdentifierInfo &II,<br>
> ParsedType ObjectType);<br>
><br>
> + /// \brief Collects information that may be used to recover erroneous nested<br>
> + /// name specifier.<br>
> + struct NameSpecRecoveryInfo {<br>
> + bool CorrectToColon; //> suggestions to replace '::' -> ':' are allowed<br>
> + bool CorrectionToColonRequested; //> Replacement '::' -> ':' recovers error<br>
> + TypeDecl *FoundDecl; //> Declaration if found for name spec<br>
<br>
Comments should be proper sentences (capitalization and punctuation).<br>
<br>
> +<br>
> + NameSpecRecoveryInfo(bool CorrectColon) {<br>
<br>
This should probably be an explicit constructor since it only takes a<br>
single bool parameter.<br>
<br>
> + clear();<br>
> + CorrectToColon = CorrectColon;<br>
<br>
Would be better to use an initializer list.<br>
<br>
> + }<br>
> + void clear() {<br>
> + CorrectToColon = CorrectionToColonRequested = false;<br>
> + FoundDecl = nullptr;<br>
> + }<br>
<br>
This method is never called from anywhere, so it should probably be removed.<br>
<br>
> + };<br>
> +<br>
> bool BuildCXXNestedNameSpecifier(Scope *S,<br>
> IdentifierInfo &Identifier,<br>
> SourceLocation IdentifierLoc,<br>
> @@ -4619,7 +4636,7 @@<br>
> CXXScopeSpec &SS,<br>
> NamedDecl *ScopeLookupResult,<br>
> bool ErrorRecoveryLookup,<br>
> - bool *IsCorrectedToColon = nullptr);<br>
> + NameSpecRecoveryInfo *RecovInfo = nullptr);<br>
<br>
As best I can tell, this does not need to be an optional parameter<br>
(the only callers of this overload of the function all pass in a<br>
nonnull pointer) -- instead, RecovInfo could be accepted by reference.<br>
(The desire for colon correction is already encoded in the<br>
NameSpecRecoveryInfo object itself.)<br>
<br>
><br>
> /// \brief The parser has parsed a nested-name-specifier 'identifier::'.<br>
> ///<br>
> @@ -4645,9 +4662,8 @@<br>
> /// \param ErrorRecoveryLookup If true, then this method is called to improve<br>
> /// error recovery. In this case do not emit error message.<br>
> ///<br>
> - /// \param IsCorrectedToColon If not null, suggestions to replace '::' -> ':'<br>
> - /// are allowed. The bool value pointed by this parameter is set to 'true'<br>
> - /// if the identifier is treated as if it was followed by ':', not '::'.<br>
> + /// \param RecovInfo If not null, points to a structure that is contains info<br>
<br>
Spurious "is".<br>
<br>
> + /// for error recovery.<br>
> ///<br>
> /// \returns true if an error occurred, false otherwise.<br>
> bool ActOnCXXNestedNameSpecifier(Scope *S,<br>
> @@ -4658,7 +4674,7 @@<br>
> bool EnteringContext,<br>
> CXXScopeSpec &SS,<br>
> bool ErrorRecoveryLookup = false,<br>
> - bool *IsCorrectedToColon = nullptr);<br>
> + NameSpecRecoveryInfo *RecovInfo = nullptr);<br>
><br>
> ExprResult ActOnDecltypeExpression(Expr *E);<br>
><br>
> Index: lib/Parse/ParseExprCXX.cpp<br>
> ===================================================================<br>
> --- lib/Parse/ParseExprCXX.cpp<br>
> +++ lib/Parse/ParseExprCXX.cpp<br>
> @@ -472,20 +472,47 @@<br>
><br>
> CheckForLParenAfterColonColon();<br>
><br>
> - bool IsCorrectedToColon = false;<br>
> - bool *CorrectionFlagPtr = ColonIsSacred ? &IsCorrectedToColon : nullptr;<br>
> + Sema::NameSpecRecoveryInfo RecovInfo(ColonIsSacred);<br>
> if (Actions.ActOnCXXNestedNameSpecifier(getCurScope(), II, IdLoc, CCLoc,<br>
> ObjectType, EnteringContext, SS,<br>
> - false, CorrectionFlagPtr)) {<br>
> + false, &RecovInfo)) {<br>
> // Identifier is not recognized as a nested name, but we can have<br>
> // mistyped '::' instead of ':'.<br>
> - if (CorrectionFlagPtr && IsCorrectedToColon) {<br>
> + if (RecovInfo.CorrectToColon && RecovInfo.CorrectionToColonRequested) {<br>
> ColonColon.setKind(tok::colon);<br>
> PP.EnterToken(Tok);<br>
> PP.EnterToken(ColonColon);<br>
> Tok = Identifier;<br>
> break;<br>
> }<br>
> + if (RecovInfo.FoundDecl) {<br>
> + // The current identifier does not represents entity that may be used<br>
<br>
"does not represent an entity"<br>
<br>
> + // as nested name spec, it nevertheless resolves to a declaration. See<br>
> + // if we can do error recover here.<br>
> + bool ErrorReported = false;<br>
> + if (EnumDecl *ED = dyn_cast<EnumDecl>(RecovInfo.FoundDecl)) {<br>
> + // Maybe non scoped enum is used as scoped? If so this identifier<br>
> + // must be followed by another, which is corresponding enumerator.<br>
> + if (!ED->isScoped() && Tok.is(tok::identifier) &&<br>
> + NextToken().isNot(tok::coloncolon)) {<br>
> + IdentifierInfo *Id = Tok.getIdentifierInfo();<br>
> + for (auto Enumerator : ED->enumerators()) {<br>
> + if (Enumerator->getName().equals(Id->getName())) {<br>
> + // We can recover parsing by throwing away enumeration name.<br>
> + Diag(IdLoc, diag::err_nested_name_spec_is_enum) <<<br>
> + FixItHint::CreateRemoval(SourceRange(IdLoc, CCLoc));<br>
> + ErrorReported = true;<br>
> + break;<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> + if (!ErrorReported) {<br>
> + Diag(IdLoc, diag::err_expected_class_or_namespace_or_enum)<br>
> + << QualType(RecovInfo.FoundDecl->getTypeForDecl(), 0)<br>
> + << getLangOpts().CPlusPlus;<br>
> + }<br>
<br>
Can elide the braces from the if statement.<br>
<br>
> + }<br>
> SS.SetInvalid(SourceRange(IdLoc, CCLoc));<br>
> }<br>
> HasScopeSpecifier = true;<br>
> Index: lib/Sema/SemaCXXScopeSpec.cpp<br>
> ===================================================================<br>
> --- lib/Sema/SemaCXXScopeSpec.cpp<br>
> +++ lib/Sema/SemaCXXScopeSpec.cpp<br>
> @@ -427,10 +427,8 @@<br>
> /// definition time.<br>
> /// \param ErrorRecoveryLookup Specifies if the method is called to improve<br>
> /// error recovery and what kind of recovery is performed.<br>
> -/// \param IsCorrectedToColon If not null, suggestion of replace '::' -> ':'<br>
> -/// are allowed. The bool value pointed by this parameter is set to<br>
> -/// 'true' if the identifier is treated as if it was followed by ':',<br>
> -/// not '::'.<br>
> +/// \param RecovInfo If not null, points to a structure that contains info<br>
> +/// for error recovery.<br>
> ///<br>
> /// This routine differs only slightly from ActOnCXXNestedNameSpecifier, in<br>
> /// that it contains an extra parameter \p ScopeLookupResult, which provides<br>
> @@ -452,15 +450,17 @@<br>
> CXXScopeSpec &SS,<br>
> NamedDecl *ScopeLookupResult,<br>
> bool ErrorRecoveryLookup,<br>
> - bool *IsCorrectedToColon) {<br>
> + NameSpecRecoveryInfo *RecovInfo) {<br>
> LookupResult Found(*this, &Identifier, IdentifierLoc,<br>
> LookupNestedNameSpecifierName);<br>
><br>
> // Determine where to perform name lookup<br>
> DeclContext *LookupCtx = nullptr;<br>
> bool isDependent = false;<br>
> - if (IsCorrectedToColon)<br>
> - *IsCorrectedToColon = false;<br>
> + if (RecovInfo) {<br>
> + RecovInfo->CorrectionToColonRequested = false;<br>
> + RecovInfo->FoundDecl = nullptr;<br>
> + }<br>
<br>
This shouldn't be required given that the constructor already clears<br>
these, and the structure is not meant to be long-lived.<br>
<br>
> if (!ObjectType.isNull()) {<br>
> // This nested-name-specifier occurs in a member access expression, e.g.,<br>
> // x->B::f, and we are looking into the type of the object.<br>
> @@ -553,8 +553,8 @@<br>
> if (!R.empty()) {<br>
> // The identifier is found in ordinary lookup. If correction to colon is<br>
> // allowed, suggest replacement to ':'.<br>
> - if (IsCorrectedToColon) {<br>
> - *IsCorrectedToColon = true;<br>
> + if (RecovInfo && RecovInfo->CorrectToColon) {<br>
> + RecovInfo->CorrectionToColonRequested = true;<br>
> Diag(CCLoc, diag::err_nested_name_spec_is_not_class)<br>
> << &Identifier << getLangOpts().CPlusPlus<br>
> << FixItHint::CreateReplacement(CCLoc, ":");<br>
> @@ -754,10 +754,13 @@<br>
> }<br>
><br>
> if (!Found.empty()) {<br>
> - if (TypeDecl *TD = Found.getAsSingle<TypeDecl>())<br>
> - Diag(IdentifierLoc, diag::err_expected_class_or_namespace)<br>
> + if (TypeDecl *TD = Found.getAsSingle<TypeDecl>()) {<br>
> + if (RecovInfo)<br>
> + RecovInfo->FoundDecl = TD;<br>
> + else<br>
> + Diag(IdentifierLoc, diag::err_expected_class_or_namespace)<br>
> << QualType(TD->getTypeForDecl(), 0) << getLangOpts().CPlusPlus;<br>
> - else {<br>
> + } else {<br>
> Diag(IdentifierLoc, diag::err_expected_class_or_namespace)<br>
> << &Identifier << getLangOpts().CPlusPlus;<br>
> if (NamedDecl *ND = Found.getAsSingle<NamedDecl>())<br>
> @@ -780,15 +783,15 @@<br>
> bool EnteringContext,<br>
> CXXScopeSpec &SS,<br>
> bool ErrorRecoveryLookup,<br>
> - bool *IsCorrectedToColon) {<br>
> + NameSpecRecoveryInfo *RecovInfo) {<br>
> if (SS.isInvalid())<br>
> return true;<br>
><br>
> return BuildCXXNestedNameSpecifier(S, Identifier, IdentifierLoc, CCLoc,<br>
> GetTypeFromParser(ObjectType),<br>
> EnteringContext, SS,<br>
> /*ScopeLookupResult=*/nullptr, false,<br>
> - IsCorrectedToColon);<br>
> + RecovInfo);<br>
> }<br>
><br>
> bool Sema::ActOnCXXNestedNameSpecifierDecltype(CXXScopeSpec &SS,<br>
> Index: test/SemaCXX/nested-name-spec.cpp<br>
> ===================================================================<br>
> --- test/SemaCXX/nested-name-spec.cpp<br>
> +++ test/SemaCXX/nested-name-spec.cpp<br>
> @@ -116,7 +116,7 @@<br>
> };<br>
><br>
> void f() {<br>
> - return E::X; // expected-error{{'E::Nested::E' is not a class, namespace, or scoped enumeration}}<br>
> + return E::X; // expected-error{{unscoped enumeration name shall not be specified in nested name specifier}}<br>
<br>
I find the original diagnostic to be more clear than the new one. I<br>
also find the test case *really* strange since f() returns void. :-P<br>
<br>
> }<br>
> }<br>
> }<br>
> @@ -410,3 +410,15 @@<br>
> };<br>
><br>
> }<br>
> +<br>
> +<br>
> +namespace PR16951 {<br>
> + namespace ns {<br>
> + enum an_enumeration {<br>
> + ENUMERATOR<br>
> + };<br>
> + }<br>
> +<br>
> + int x1 = ns::an_enumeration::ENUMERATOR; // expected-error{{unscoped enumeration name shall not be specified in nested name specifier}}<br>
> + int x2 = ns::an_enumeration::ENUMERATOR::vvv; // expected-error{{'PR16951::ns::an_enumeration' is not a class, namespace, or scoped enumeration}}<br>
> +}<br>
><br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div></div>