Skip to content
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

[Parser][QoI] Offer fixit for changing the platform condition kind #25977

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@roop
Copy link

commented Jul 5, 2019

Resolves SR-11037.

In case there’s an exact match with a value of an alternate kind, this solution offers a single fixit to change the platform conditional kind. For example:

#if os(simulator)
// Fixit: os -> targetEnvironment
#endif

In case there’s only a case-insensitive match with a value of an alternate kind, this solution offers two fixits: one for the kind and one for the value. For example:

#if os(Simulator)
// Fixit: os -> targetEnvironment
// Fixit: Simulator -> simulator
#endif

Implementation details:

To search for a match in an alternate platform conditional kind, we need a way to loop through the different platform conditional value arrays (SupportedConditionalCompilationOSs, SupportedConditionalCompilationArches, …), for which I’m using a std::pair<const StringRef*, size_t> to represent each array. I’m open to suggestions on an alternate representation.

[Parser][QoI] Offer fixit for changing the platform condition kind
For example, for "#if os(simulator)", offer a fixit to change
"os" to "targetEnvironment", instead of offering to change "simulator".

Resolves SR-11037.
@theblixguy

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

@swift-ci please test

(Sorry about the delay—I've been a little busy.)

@brentdax brentdax self-requested a review Jul 11, 2019

@brentdax
Copy link
Collaborator

left a comment

I like where this is going!

Show resolved Hide resolved lib/Parse/ParseIfConfig.cpp
Show resolved Hide resolved lib/Basic/LangOptions.cpp
#if _endian(arm64) // expected-warning {{unknown endianness for build configuration '_endian'}} expected-note {{did you mean 'arch'}} {{5-12=arch}}
#endif

#if targetEnvironment(_ObjC) // expected-warning {{unknown target environment for build configuration 'targetEnvironment'}} expected-note {{did you mean '_runtime'}} {{5-22=_runtime}}

This comment has been minimized.

Copy link
@brentdax

brentdax Jul 11, 2019

Collaborator

Come to think of it, we probably shouldn't suggest underscored kinds—they're not officially supported, after all.

This comment has been minimized.

Copy link
@roop

roop Jul 17, 2019

Author

Makes sense. This is addressed in commit e44a458.

@roop

This comment has been minimized.

Copy link
Author

commented Jul 13, 2019

I’m afraid I will not be working on this for a few more days. I hope to continue working on this on Tue 16/Jul. Sorry about the delay.

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

@roop I started as a volunteer contributor—I know how it is. Thanks for tackling this!

@brentdax brentdax self-requested a review Jul 17, 2019

@brentdax
Copy link
Collaborator

left a comment

Much better! Just a couple of nitpicks.

Show resolved Hide resolved include/swift/AST/PlatformConditionKinds.def Outdated
Show resolved Hide resolved include/swift/Basic/LangOptions.h
Show resolved Hide resolved lib/Basic/LangOptions.cpp
@roop

This comment has been minimized.

Copy link
Author

commented Jul 18, 2019

Ideally, the previous 2 commits should've been an update to the "Make a .def file for PlatformConditionKind" commit, but I'm not sure of the Swift project's policy on force-pushing to PRs under review, so I'm playing it safe and just adding commits on top.

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

@roop Force-pushes in PRs are just fine. They do confuse the CI infrastructure a bit, though—you get an "error" which is really just the job being rescheduled with a different commit hash.

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

@swift-ci please test

@swift-ci

This comment was marked as resolved.

Copy link
Contributor

commented Jul 18, 2019

Build failed
Swift Test Linux Platform
Git Sha - b1ee2d4

@swift-ci

This comment was marked as resolved.

Copy link
Contributor

commented Jul 18, 2019

Build failed
Swift Test OS X Platform
Git Sha - b1ee2d4

roop added some commits Jul 12, 2019

[Parser][QoI] Suppress fixits to underscored platform condition kinds
"_endian" and "_runtime" aren't officially supported platform condition
kinds, so don't suggest changing to one of those.

@roop roop force-pushed the roop:sr11037_fixit_platform_condition_kind branch from 28692e9 to fb0c2e7 Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.