Skip to content

Adjust printing to take into account special DeclNames #9979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 25, 2017
Merged

Adjust printing to take into account special DeclNames #9979

merged 1 commit into from
Jun 25, 2017

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented May 30, 2017 •
edited
Loading

Print DeclBaseNames using a new userFacingStr() method to prepare for
DeclBaseNames that are not backed by Identifiers

This includes the changes from #9975, #9976, #9977, #9978, #9978

@ahoppen ahoppen requested a review from jrose-apple May 30, 2017 13:24
@ahoppen
Copy link
Member Author

ahoppen commented May 30, 2017

Please test with following pull request:
apple/swift-lldb#206

@swift-ci Please test

jrose-apple
jrose-apple reviewed May 30, 2017
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looking good.

include/swift/AST/Identifier.h Outdated
@@ -233,6 +233,12 @@ class DeclBaseName {
return !isSpecial() && getIdentifier().isEditorPlaceholder();
}

/// A representation of the name to be displayed to users. May be ambiguous
/// between identifiers and special names
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Missing period.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

include/swift/AST/Identifier.h Outdated
@@ -233,6 +233,12 @@ class DeclBaseName {
return !isSpecial() && getIdentifier().isEditorPlaceholder();
}

/// A representation of the name to be displayed to users. May be ambiguous
/// between identifiers and special names
StringRef userFacingStr() const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I feel like the abbreviation of "str" doesn't make sense in a longer name. How about "userFacingName" or "nameForDiagnostics" or something like that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go with userFacingName since it's not only used for diagnostics but also for some other stuff like sorting names.

lib/AST/ASTVerifier.cpp Outdated
@@ -3202,7 +3202,8 @@ bool swift::shouldVerify(const Decl *D, const ASTContext &Context) {
return true;
}

size_t Hash = llvm::hash_value(VD->getNameStr());
size_t Hash =
llvm::DenseMapInfo<DeclBaseName>::getHashValue(VD->getBaseName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a hash that's stable from run to run. (The AST verifier doesn't need to be fast.)

lib/AST/Identifier.cpp Outdated
// TODO: Handle special names
return OS << I.getIdentifier();
if (I.empty())
return OS << "_";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think << should behave differently from userFacingStr (or whatever). Either they should both insert the "_" or neither should.

Copy link
Member Author

@ahoppen ahoppen May 31, 2017 •
edited by jrose-apple
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes sense. userFacingStr now returns "_" for the empty name since we require "_" for << for quite a few diagnostics IIRC

lib/IDE/CodeCompletion.cpp
@@ -3729,7 +3729,8 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
for (auto Ex : NTD->getExtensions()) {
handleDeclRange(Ex->getMembers(), Reason);
}
} else if (isNameHit(VD->getNameStr())) {
} else if (!VD->getBaseName().isSpecial() &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benlangmuir, is this method ever supposed to produce subscripts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably should. The situation would be something like
x[label: #^COMPLETE_HERE^#]

lib/IDE/SyntaxModel.cpp Outdated
DRE->getName().getBaseIdentifier().getLength())))
if (!Fn(CharSourceRange(
DRE->getSourceRange().Start,
DRE->getName().getBaseName().userFacingStr().size())))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for @nathawes and @bitjammer: this is not a good way to measure the length of an identifier because it won't handle the backtick-escaping syntax. Using the lexer is the right way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted!

@ahoppen ahoppen mentioned this pull request May 30, 2017
Merged
@ahoppen ahoppen force-pushed the adjust-declname-printing branch from 2952410 to ac44647 Compare May 31, 2017 11:45
@ahoppen
Copy link
Member Author

ahoppen commented May 31, 2017

@jrose-apple I renamed userFacingStr to userFacingName and took care of your other comments. Could you maybe another look through?

@swift-ci please test

@ahoppen ahoppen force-pushed the adjust-declname-printing branch from ac44647 to 638bf80 Compare May 31, 2017 14:03
@ahoppen
Copy link
Member Author

ahoppen commented May 31, 2017

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - ac44647744f7add5b65f828a337f8157aa9f4073
Test requested by - @ahoppen

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - ac44647744f7add5b65f828a337f8157aa9f4073
Test requested by - @ahoppen

jrose-apple
jrose-apple reviewed May 31, 2017
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. Still need to get back to the diagnostic changes.

@ahoppen
Adjust printing to take into account special DeclNames
949968a 
Print DeclBaseNames using a new userFacingStr() method to prepare for
DeclBaseNames that are not backed by Identifiers
@ahoppen ahoppen force-pushed the adjust-declname-printing branch from 638bf80 to 949968a Compare June 24, 2017 09:40
@ahoppen
Copy link
Member Author

ahoppen commented Jun 24, 2017

@swift-ci Please test

@swiftlang swiftlang deleted a comment from swift-ci Jun 24, 2017
@swiftlang swiftlang deleted a comment from swift-ci Jun 24, 2017
@ahoppen
Copy link
Member Author

ahoppen commented Jun 24, 2017

macOS failure seemed unrelated. Let's try again

@swift-ci Please test OS X Platform

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 949968a
Test requested by - @ahoppen

@ahoppen
Copy link
Member Author

ahoppen commented Jun 25, 2017

Unrelated failure seems to have been resolved in #10567

@swift-ci Please test OS X Platform

@ahoppen ahoppen merged commit e7a0ba8 into swiftlang:master Jun 25, 2017
@ahoppen ahoppen deleted the adjust-declname-printing branch June 26, 2017 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benlangmuir benlangmuir benlangmuir left review comments

@jrose-apple jrose-apple jrose-apple left review comments

@nathawes nathawes nathawes left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ahoppen @swift-ci @benlangmuir @jrose-apple @nathawes