Skip to content

[SyntaxParse] Re-apply move-only ParsedRawSyntaxNode #27230

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 2 commits into from
Sep 19, 2019
Merged

[SyntaxParse] Re-apply move-only ParsedRawSyntaxNode #27230

merged 2 commits into from
Sep 19, 2019

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Sep 18, 2019

Revert #27203 with a fix.

Locally confirmed this successfully compiles stdlib with asan+ubsan in both debug and release build.

rdar://problem/55421369

@rintaro
Revert "Merge pull request swiftlang#27203 from CodaFi/movin-on-up"
9f642f0 
This reverts commit 387d2a9, reversing
changes made to bf1ab6c.
@rintaro
[SyntaxParses] Address memory issue in popTokenSyntax()
4527711 
Don't return reference for local temporary object

rdar://problem/55421369
@rintaro
Copy link
Member Author

rintaro commented Sep 18, 2019

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Sep 18, 2019

@swift-ci Please ASAN test

rintaro
rintaro commented Sep 18, 2019
View reviewed changes
lib/Parse/SyntaxParsingContext.cpp
@@ -163,7 +164,8 @@ const SyntaxParsingContext *SyntaxParsingContext::getRoot() const {
}

ParsedTokenSyntax SyntaxParsingContext::popToken() {
return popIf<ParsedTokenSyntax>().getValue();
auto tok = popIf<ParsedTokenSyntax>();
return std::move(tok.getValue());
Copy link
Member Author

@rintaro rintaro Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In #27190, this was:

ParsedTokenSyntax &&SyntaxParsingContext::popToken() {
  return std::move(popIf<ParsedTokenSyntax>().getValue());
}

This returns a reference to a field of the temporary local value of popIf<ParsedTokenSyntax>() (Optional<ParsedTokenSyntax>).

Copy link
Member Author

Choose a reason for hiding this comment

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

@compnerd Could you check if this change doesn't break Windows build?

@rintaro
Copy link
Member Author

rintaro commented Sep 18, 2019

https://ci.swift.org/job/swift-PR-osx-ASAN-test/96/ Infrastructure issue?

@rintaro
Copy link
Member Author

rintaro commented Sep 18, 2019

@swift-ci Please ASAN test

@CodaFi
Copy link
Contributor

CodaFi commented Sep 18, 2019

The ASAN bot builds in release with debug info. It shouldn't catch this.

@rintaro
Copy link
Member Author

rintaro commented Sep 18, 2019 •
edited
Loading

@CodaFi As I noted in the description, I confirmed this change successfully compiles stdlib with asan+ubsan in both debug and release build in my local machine.

@shahmishal Is there a way to invoke pull request CI with debug+ASAN ?

@jrose-apple
Copy link
Contributor

@swift-ci Please test Windows

@rintaro
Copy link
Member Author

rintaro commented Sep 18, 2019

preset=buildbot_incremental,tools=DA,stdlib=DA,build
@swift-ci Please test with preset macOS Platform

@rintaro rintaro merged commit b09f875 into swiftlang:master Sep 19, 2019
rintaro added a commit to rintaro/swift that referenced this pull request Oct 14, 2019
@rintaro
Revert "Merge pull request swiftlang#27230 from rintaro/syntaxparsse-…
00613db 
…rdar55421369"

This reverts commit b09f875, reversing
changes made to d0b7eca.
rintaro added a commit to rintaro/swift that referenced this pull request Oct 14, 2019
@rintaro
Revert "Revert "Merge pull request swiftlang#27230 from rintaro/synta…
9db6464 
…xparsse-rdar55421369""

This reverts commit 00613db.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rintaro @CodaFi @jrose-apple