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