Page MenuHomePhabricator

[Parser][ObjC] Improve diagnostics and recovery when C++ keywords are used as identifiers in Objective-C++
ClosedPublic

Authored by arphaman on Nov 10 2016, 7:20 AM.

Details

Summary

This patch improves the 'expected identifier' errors that are presented when a C++ keyword is used as an identifier in Objective-C++ by mentioning that this is a C++ keyword in the diagnostic message. It also improves the error recovery: the parser will treat the C++ keywords as identifiers to prevent unrelated parsing errors.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 77482.Nov 10 2016, 7:20 AM
arphaman retitled this revision from to [Parser][ObjC] Improve diagnostics and recovery when C++ keywords are used as identifiers in Objective-C++.
arphaman updated this object.
arphaman added reviewers: manmanren, bruno.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: cfe-commits.
manmanren edited edge metadata.Nov 15 2016, 3:26 PM

Cheers,
Manman

include/clang/Basic/DiagnosticParseKinds.td
442 ↗(On Diff #77482)

This name is a little misleading. It sounds like we are expecting a keyword :]

include/clang/Parse/Parser.h
798 ↗(On Diff #77482)

--> if the next token is a C++ (if to is)

801 ↗(On Diff #77482)

Same here. The function name is kind of misleading.

lib/Parse/ParseDecl.cpp
5405 ↗(On Diff #77482)

Does C++ have the same issue? Or is this only needed for Objective-C++?

lib/Parse/ParseObjc.cpp
153 ↗(On Diff #77482)

Are we doing this for all occurrences of "isNot(token::identifier)"?
Is it better to wrap this "&&" in the function and rename the function to something like diagnoseNonIdentifier?

arphaman updated this revision to Diff 78218.Nov 16 2016, 10:34 AM
arphaman edited edge metadata.
arphaman marked 4 inline comments as done.

Addressed review comments by renaming the diagnostic and simplifying the name and the use of the expectIdentifier method.

arphaman added inline comments.Nov 16 2016, 10:36 AM
lib/Parse/ParseDecl.cpp
5405 ↗(On Diff #77482)

I suppose it kinda does have it as well, but Objective-C++ has worse error recovery for Objective-C declarations in this case, so it's more important for it. I wouldn't mind adding a similar diagnostic for C++ though, but I think that should be done in a separate patch.

manmanren accepted this revision.Dec 7 2016, 8:23 AM
manmanren edited edge metadata.

LGTM

Manman

This revision is now accepted and ready to land.Dec 7 2016, 8:23 AM
This revision was automatically updated to reflect the committed changes.