Skip to content

Update the compiler arguments used for background AST builds #1971

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.

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

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

bnbarham
Copy link
Contributor

@bnbarham bnbarham commented Feb 6, 2025

This fixes two issues:

  1. The SwiftPM build system was setup without passing through whether it should prepare or not. This meant that we lost eg. the argument to allow compiler errors when building the AST (even though it was set when building the modules)
  2. The compiler argument adjustment to remove harmful and unnecessary flags only applied to indexing arguments, not those passed to the AST builds

Resolves rdar://141508656.

@bnbarham bnbarham requested a review from ahoppen as a code owner February 6, 2025 02:08
@bnbarham
Copy link
Contributor Author

bnbarham commented Feb 6, 2025

@swift-ci please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines 1303 to 1304
case .removeOptionAndPreviousArgument:
_ = result.popLast()
Copy link
Member

Choose a reason for hiding this comment

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

Should we validate that the previous argument is indeed -Xfrontend? If it is not, should we log a fault and not remove the argument? Probably best to give removeOptionAndPreviousArgument an associated value with the expected previous argument in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I skipped the log, seems noisy and if the flag is there then we should remove it regardless (but have skipped removing the previous).

@bnbarham bnbarham force-pushed the pass-allow-errors-for-ast branch from baf8d12 to 29b4e95 Compare February 6, 2025 23:14
@bnbarham
Copy link
Contributor Author

bnbarham commented Feb 6, 2025

@swift-ci please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

One more small comment. Otherwise, I think you need to update CMakeLists.txt since you moved files.

@bnbarham bnbarham force-pushed the pass-allow-errors-for-ast branch from 29b4e95 to 25548d5 Compare February 7, 2025 02:05
@bnbarham
Copy link
Contributor Author

bnbarham commented Feb 7, 2025

@swift-ci please test

@bnbarham
Copy link
Contributor Author

bnbarham commented Feb 7, 2025

@swift-ci please test Windows platform

This fixes two issues:
1. The SwiftPM build system was setup without passing through whether it
   should prepare or not. This meant that we lost eg. the argument to
   allow compiler errors when building the AST (even though it was set
   when building the modules)
2. The compiler argument adjustment to remove harmful and unnecessary
   flags only applied to indexing arguments, not those passed to the AST
   builds

Resolves rdar://141508656.
@bnbarham bnbarham force-pushed the pass-allow-errors-for-ast branch from 25548d5 to ab12429 Compare February 7, 2025 19:57
@bnbarham
Copy link
Contributor Author

bnbarham commented Feb 7, 2025

@swift-ci please test

@bnbarham
Copy link
Contributor Author

bnbarham commented Feb 7, 2025

@swift-ci please test Windows platform

@bnbarham bnbarham merged commit d5978d5 into swiftlang:main Feb 8, 2025
3 checks passed
@bnbarham bnbarham deleted the pass-allow-errors-for-ast branch February 8, 2025 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants